Closed Bug 1158822 Opened 5 years ago Closed 5 years ago

Make the links in the markup-view that point to scripts and stylesheets open the appropriate tool

Categories

(DevTools :: Inspector, defect, P1)

defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: pbro, Assigned: pbro)

References

Details

(Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(3 files, 1 obsolete file)

Bug 921102 will make it possible to open links displayed in attributes in the markup-view.
Some of these links are URLs of css and js resources, so these links should open the style-editor or debugger, at the right location.
Depends on: 921102
Jeff, bug 921102 only makes markup-view links to open new tab, this bug is about making script/style links open the debugger/style-editor. Should this be flag with devedition-40?
Flags: needinfo?(jgriffiths)
Yes!
Flags: needinfo?(jgriffiths)
Priority: -- → P1
Whiteboard: [devedition-40]
Whiteboard: [devedition-40] → [devedition-40][difficulty=medium]
To open the debugger, there's a bit of code to write, and an example is available in Tooltip.js/EventTooltip.prototype._debugClicked
We should factor this out into a reusable function.
And there's an example of how to open a file in the style-editor here: computed-view.js/SelectorView.prototype.openStyleEditor
Bug 921102 is nearing completion, so I've started working on this in parallel, and I have patches that work already.
I need to polish them up and add tests, but they're pretty straightforward, so it should be ok for devedition-40.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
/r/7931 - Bug 1158822 - 1 - Make sure Style-Editor rejects when selecting an non-existant sheet
/r/7933 - Bug 1158822 - 2 - Add 2 toolbox methods to open scripts and styles in appropriate tools
/r/7935 - Bug 1158822 - 3 - Link css url attributes to style-editor and js url attributes to debugger

Pull down these commits:

hg pull -r 086856c29eaf2d65ee2d428703a455e7972b2709 https://reviewboard-hg.mozilla.org/gecko/
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> hg pull -r 086856c29eaf2d65ee2d428703a455e7972b2709
> https://reviewboard-hg.mozilla.org/gecko/
I haven't flagged anyone for review yet because I want bug 921102 to be reviewed first (as it might impact the patches here), but these patches now have tests and should be close to landing I think.
I'll test them on try in the meantime.
Attachment #8599888 - Flags: review?(jsantell)
Comment on attachment 8599888 [details]
MozReview Request: bz://1158822/pbrosset

/r/7931 - Bug 1158822 - Link css url attributes to style-editor and js url attributes to debugger

Pull down this commit:

hg pull -r fcca4fe73c147381c1fcccda4f7956c8d41b4c2b https://reviewboard-hg.mozilla.org/gecko/
Hey Jordan, not sure how much you know about the inspector, feel free to redirect the review to Brian or Mike if you don't think you can review this.
This is only a small-ish change on top of bug 921102 (instead of handling links to URIs and js/css resource URIs the same way in the markup-view, now js/css resource URIs open the debugger/style-editor).
The main change in this patch is using the new toolbox viewSourceInStyleEditor and viewSourceInDebugger methods which you introduced recently.

Pending try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d7733fef49b
Comment on attachment 8599888 [details]
MozReview Request: bz://1158822/pbrosset

https://reviewboard.mozilla.org/r/7929/#review6819

::: browser/devtools/inspector/inspector-panel.js:794
(Diff revision 2)
> +            "inspector.menu.openFileInStyleEditor.label"));

Maybe we should move these into a general "toolbox" localization file? I imagine they're used everywhere else that the view source in toolbox methods are. (Maybe file a follow up bug for this)

::: browser/devtools/inspector/inspector-panel.js:1122
(Diff revision 2)
>        }, console.error);

Change the wrapper promise to only have a resolve fn, and add a catch to the end of that, and have the viewSourceIn* methods return. That way we don't need to have `catch` there as well

this.inspector.resolveRelativeURL(..).then(url => {
  ...
  return this.toolbox.viewSourceInDebugger();
}).catch(console.error)

::: browser/devtools/markupview/test/browser_markupview_links_06.js:24
(Diff revision 2)
> +  let onStyleEditorReady = toolbox.once("styleeditor-ready");

Should be aware of bug 1160305, which is an intermittent in the viewSourceInStyleEditor test that baffles me. You can check that the SE has the correct file open etc using the logic here, but something's wrong with that test, so maybe not a good idea.

::: browser/devtools/shared/node-attribute-parser.js:88
(Diff revision 2)
> -  {namespaceURI: "*", attributeName: "src", tagName: "script", type: TYPE_RESOURCE_URI},
> +  {namespaceURI: "*", attributeName: "src", tagName: "script", type: TYPE_JS_RESOURCE_URI},

Are there any other scenarios where there'd be linkable CSS/JS file? Maybe some elements in the <head>, or like requirejs's loader requires an element with a field specifying the entry point. I can't think of anything specifically that's standardized, atleast.
Attachment #8599888 - Flags: review?(jsantell)
Comment on attachment 8599888 [details]
MozReview Request: bz://1158822/pbrosset

Just a nit with the promise catching, and maybe a follow up on sharing "open source in.." strings (follow up bug probably), and everything else is just extra info. Looks good!
Attachment #8599888 - Flags: review+
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #11)
> Comment on attachment 8599888 [details]
> MozReview Request: bz://1158822/pbrosset
> 
> Just a nit with the promise catching, and maybe a follow up on sharing "open
> source in.." strings (follow up bug probably), and everything else is just
> extra info. Looks good!
Thanks Jordan. I have moved the strings to toolbox.properties and made the key names a little bit more generic so that they can be used in the UI everywhere viewSourceInStyleEditor/Debugger are used.
I've also fixed the promise wrapping logic.
Thanks for mentioning bug 1160305, I'll keep an eye out for intermittent failures of the new test I'm adding.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #10)
> Are there any other scenarios where there'd be linkable CSS/JS file? Maybe
> some elements in the <head>, or like requirejs's loader requires an element
> with a field specifying the entry point. I can't think of anything
> specifically that's standardized, atleast.
Yeah, right now, the list of standard attributes should be complete, but it's very easy to add parsing for new ones if there are popular non-standard linkable attributes.
I have made the changes according to review comments.
Last try was green, and these last changes shouldn't change this, so:
https://hg.mozilla.org/integration/fx-team/rev/532326604adc
https://hg.mozilla.org/mozilla-central/rev/532326604adc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Attachment #8599888 - Attachment is obsolete: true
Attachment #8620161 - Flags: review+
Attachment #8620162 - Flags: review+
Attachment #8620163 - Flags: review+
Whiteboard: [devedition-40][difficulty=medium] → [polish-backlog][difficulty=medium]
Duplicate of this bug: 871924
Duplicate of this bug: 1020242
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.