870004 - "Remotable Style Editor" broke stylesheet links from the Inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[David Creswick]

Bug 816967 (remotable style editor) changed the API of StyleEditorPanel#selectStyleSheet, but didn't change the calls to it in the inspector. I am a little surprised that a test didn't catch this.

Comment 1

[Heather Arthur [:harth]]

Oooh. There are two tests for this: browser_computedview_734259_style_editor_link.js and browser/devtools/styleinspector/test/browser_ruleview_734259_style_editor_link.js, but it looks like they don't test much, just that the style editor is opened.

Comment 2

[Rob Campbell [:rc] (:robcee)]

dcrew: This is a regression and would like to get this in before the merge on Monday. Any chance of a patch this weekend or should harth take it?

Comment 3

[David Creswick]

Someone else can take it. The fix itself is very simple, but updating the tests to catch it next time is not as simple.

Comment 4

[David Creswick]

Attachment: patch v1

So I did end up working on this after all.

Comment 5

[Heather Arthur [:harth]]

Comment on attachment 748453 [details] [diff] [review]
patch v1

Review of attachment 748453 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into this. This looks good. One thing I realized is that keying on the stylesheet's href doesn't work for multiple inline style sheets, as they all have an href of undefined. We might have to key off of their styleSheetIndex.

If that's an easy fix, update the patch. Otherwise, this is certainly better than broken.

Comment 6

[Heather Arthur [:harth]]

I'm going to check this in, and barring any backouts, file a follow up bug about the inline stylesheets.

Comment 7

[Heather Arthur [:harth]]

https://hg.mozilla.org/integration/fx-team/rev/b906018611c8

Comment 8

[Heather Arthur [:harth]]

Filed bug 871423

Comment 9

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/mozilla-central/rev/b906018611c8

Comment 10

[David Creswick]

This should probably be pushed to firefox 23, given that it's a regression. How does one go about doing that?

Comment 11

[Anton Kovalyov (:anton)]

I was going to do an aurora-approval request but then I realized that this patch can't be applied cleanly to the Aurora codebase. Somebody (David? Heather?) will need to check out mozilla-aurora, rebase the patch, get r+ and then file a?

I (or Heather) can check it in directly to the Aurora channel after that.

Also, reopening the bug since we're tracking Firefox 23 now.

Comment 12

[Heather Arthur [:harth]]

Comment on attachment 748453 [details] [diff] [review]
patch v1

This patch applies for me on the latest from http://hg.mozilla.org/releases/mozilla-aurora/

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 816967

User impact if declined:
Clicking on a link in the the Style Inspector would open a blank Style Editor with no sheet selected, clearly broken behavior.

Testing completed (on m-c, etc.):
In m-c

Risk to taking this patch (and alternatives if risky): 
Low risk, only touches devtools code.

String or IDL/UUID changes made by this patch:
None

Comment 13

[Anton Kovalyov (:anton)]

Pushed to Aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/bca5f2d86152

Comment 14

[Ryan VanderMeulen [:RyanVM]]

FYI, we typically don't reopen bugs for branch uplifts. The bug resolution is used to track when a patch lands on mozilla-central. The tracking flags (like I just set for you here) are for branch uplifts.