Bug 112689 – Web Inspector: factor out node search controller from inspector.js

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Andrey Kosyakov]

This extracts the node search logic from inspector.js into a separate file, so it's easier to extend.

[Andrey Kosyakov]

Created attachment 193810 [details]
Patch

[Pavel Feldman]

Comment on attachment 193810 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193810&action=review

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:47
> +    return WebInspector.KeyboardShortcut.makeDescriptor("f", WebInspector.KeyboardShortcut.Modifiers.CtrlOrMeta | WebInspector.KeyboardShortcut.Modifiers.Shift);

It was Alt

> Source/WebCore/inspector/front-end/InspectorFrontendAPI.js:110
> +        if (WebInspector.nodeSearchController)

This is now called inspectElementMode

[Andrey Kosyakov]

Created attachment 193854 [details]
Patch

[Andrey Kosyakov]

(In reply to comment #2)
> (From update of attachment 193810 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193810&action=review
> 
> > Source/WebCore/inspector/front-end/AdvancedSearchController.js:47
> > +    return WebInspector.KeyboardShortcut.makeDescriptor("f", WebInspector.KeyboardShortcut.Modifiers.CtrlOrMeta | WebInspector.KeyboardShortcut.Modifiers.Shift);
> 
> It was Alt

Oops! Thanks, rolled this part out.

> > Source/WebCore/inspector/front-end/InspectorFrontendAPI.js:110
> > +        if (WebInspector.nodeSearchController)
> 
> This is now called inspectElementMode

Renamed to elementInspectionController.

[WebKit Review Bot]

Comment on attachment 193854 [details]
Patch

Attachment 193854 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17192608

[Early Warning System Bot]

Comment on attachment 193854 [details]
Patch

Attachment 193854 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17213648

[WebKit Review Bot]

Comment on attachment 193854 [details]
Patch

Attachment 193854 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17242092

[Early Warning System Bot]

Comment on attachment 193854 [details]
Patch

Attachment 193854 [details] did not pass qt-wk2-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17181606

[Peter Beverloo (cr-android ews)]

Comment on attachment 193854 [details]
Patch

Attachment 193854 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17196514

[Andrey Kosyakov]

Created attachment 193860 [details]
Patch

[Pavel Feldman]

Comment on attachment 193860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193860&action=review

> Source/WebCore/WebCore.vcproj/WebCore.vcproj:77701
> +					RelativePath="..\inspector\front-end\ElementInspectionController.js"

InspectElementModeController?

> Source/WebCore/inspector/front-end/ElementInspectionController.js:34
> +    this.toggleSearchButton = new WebInspector.StatusBarButton(WebInspector.UIString("Select an element in the page to inspect it."), "node-search-status-bar-item");

Lets hide it.

> Source/WebCore/inspector/front-end/inspector.js:97
> +            mainStatusBar.insertBefore(this.elementInspectionController.toggleSearchButton.element, bottomStatusBarContainer);

You should encapsulate (hide) toggleSearchButton.

> Source/WebCore/inspector/front-end/inspector.js:677
> +    section.addKey(elementInspectionShortcut, WebInspector.UIString("Select node to inspect"));

Why is "Select node to inspect" string here?

> Source/WebCore/inspector/front-end/inspector.js:969
> +    if (WebInspector.elementInspectionController && WebInspector.elementInspectionController.enabled()) {

Can it be missing?

[Andrey Kosyakov]

Created attachment 194038 [details]
Patch

[Andrey Kosyakov]

(In reply to comment #11)
> View in context: https://bugs.webkit.org/attachment.cgi?id=193860&action=review
> 
> > Source/WebCore/WebCore.vcproj/WebCore.vcproj:77701
> > +					RelativePath="..\inspector\front-end\ElementInspectionController.js"
> 
> InspectElementModeController?

Done.

> > Source/WebCore/inspector/front-end/ElementInspectionController.js:34
> > +    this.toggleSearchButton = new WebInspector.StatusBarButton(WebInspector.UIString("Select an element in the page to inspect it."), "node-search-status-bar-item");
> 
> Lets hide it.

Done.

> > Source/WebCore/inspector/front-end/inspector.js:97
> > +            mainStatusBar.insertBefore(this.elementInspectionController.toggleSearchButton.element, bottomStatusBarContainer);
> 
> You should encapsulate (hide) toggleSearchButton.

Done.

> > Source/WebCore/inspector/front-end/inspector.js:677
> > +    section.addKey(elementInspectionShortcut, WebInspector.UIString("Select node to inspect"));
> 
> Why is "Select node to inspect" string here?

This is done consistently with other shortcuts there. Am I missing something?

> > Source/WebCore/inspector/front-end/inspector.js:969
> > +    if (WebInspector.elementInspectionController && WebInspector.elementInspectionController.enabled()) {
> 
> Can it be missing?

Yes, worker inspectors don't have it.

[Pavel Feldman]

Comment on attachment 194038 [details]
Patch

Wrong patch?

[Andrey Kosyakov]

Created attachment 194593 [details]
Patch

[Andrey Kosyakov]

(In reply to comment #14)
> (From update of attachment 194038 [details])
> Wrong patch?

Yup, sorry for the mess. Fixed!

[Andrey Kosyakov]

Committed r147105: <http://trac.webkit.org/changeset/147105>