Page MenuHomePhabricator

Bug 1811004 - [devtools] Make the search modifiers a shared component r=ochameau
ClosedPublic

Authored by bomsy on Feb 6 2023, 8:56 AM.
Referenced Files
Unknown Object (File)
Thu, May 1, 7:02 AM
Unknown Object (File)
Thu, Apr 17, 9:52 AM
Unknown Object (File)
Wed, Apr 16, 10:51 PM
Unknown Object (File)
Apr 9 2025, 10:23 PM
Unknown Object (File)
Mar 11 2025, 6:14 PM
Unknown Object (File)
Mar 1 2025, 7:03 AM
Unknown Object (File)
Feb 16 2025, 7:29 AM
Unknown Object (File)
Feb 1 2025, 7:32 AM
Subscribers

Details

Summary

This patch makes the search modifiers a shared component so we can
use this for project search.

Diff Detail

Repository
rMOZILLACENTRAL mozilla-central
Lint
Lint Not Applicable
Unit
Tests Not Applicable
Build Status
Buildable 509406
Build 605810: arc lint + arc unit

Event Timeline

phab-bot published this revision for review.Feb 6 2023, 8:56 AM
phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".
phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".
phab-bot removed a project: secure-revision.

Code analysis found 2 defects in the diff 677466:

  • 2 defects found by file-whitespace (Mozlint)
IMPORTANT: Found 2 issues (error level) that must be fixed before landing.

You can run this analysis locally with:

  • ./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 677466.

It looks good codewise, but for some reason there is some issue with the icon, at least on linux:

image.png (225×500 px, 16 KB)

devtools/client/debugger/src/components/Editor/SearchBar.js
257–262

You can simplify the interface between these two components by doing this.
SearchModifiers doesn't need the query nor does it need two callbacks.

May be I applied the patch the wrong way, but I'm no longer seeing any icon:

image.png (214×615 px, 12 KB)

This revision now requires changes to proceed.Feb 13 2023, 11:15 AM
bomsy requested review of this revision.Feb 13 2023, 1:57 PM
bomsy updated this revision to Diff 680305.

Code analysis found 2 defects in the diff 680305:

  • 2 defects found by file-whitespace (Mozlint)
IMPORTANT: Found 2 issues (error level) that must be fixed before landing.

You can run this analysis locally with:

  • ./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 680305.

bomsy updated this revision to Diff 681467.

2 issues closed compared to the previous diff 681491.


If you see a problem in this automated review, please report it here.

This looks great, thanks!

devtools/client/debugger/src/components/Editor/SearchBar.js
259

follow-up: To followup on our discussions around cx and the middleware asserting it...
I really doubt the usefulness and even challenge the correctness of passing cx for such action!

We do pass cx for this action:
https://searchfox.org/mozilla-central/rev/b579290e6b830d1b3f0a941203b0c0e1e56c42a3/devtools/client/debugger/src/actions/file-search.js#61-63

The main reason to pass cx in actions is to have this middleware to stop the action if the context changed:
https://searchfox.org/mozilla-central/rev/b579290e6b830d1b3f0a941203b0c0e1e56c42a3/devtools/client/debugger/src/actions/utils/middleware/context.js#19,27
And this will typically throws and prevent the action from reaching the reducers if we navigated:
https://searchfox.org/mozilla-central/rev/b579290e6b830d1b3f0a941203b0c0e1e56c42a3/devtools/client/debugger/src/utils/context.js#52-58
https://searchfox.org/mozilla-central/rev/b579290e6b830d1b3f0a941203b0c0e1e56c42a3/devtools/client/debugger/src/reducers/pause.js#308-313

But here... imagine for some weird reason we do navigate while toggling modifiers.
If the use clicked on the modifiers, we do want to unconditionaly toggle the state in the reducer.

It is really unclear this particular action have been affected by https://bugzilla.mozilla.org/show_bug.cgi?id=1535362 ...

Also, the middleware assertion is especially useful when things are asynchronous.
Here the action is synchronous, it isn't even a thunk action.
And we might assume that React components are somewhat synchronously correct? But I could imagine some ways where React is somewhat async and it may, as async thunk action, benefit from the middleware assertion...

This revision is now accepted and ready to land.Feb 16 2023, 11:07 AM