Closed Bug 939098 Opened 6 years ago Closed 6 years ago

Output parser parses color names out of context in markup panel

Categories

(DevTools :: Inspector, defect)

27 Branch
defect
Not set

Tracking

(firefox26 unaffected, firefox27 fixed, firefox28 fixed)

RESOLVED FIXED
Firefox 28
Tracking Status
firefox26 --- unaffected
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: miker, Assigned: miker)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

So in the markup panel:
class="someclass background someotherclass red"

Is converted to:
class="someclass #6363CE someotherclass #F00"
Status: NEW → ASSIGNED
Comment on attachment 832939 [details] [diff] [review]
color-names-out-of-context-939098.patch

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

When would we want onlyParseColorsAfterSupportingCSSProperty=false ?
(In reply to Joe Walker [:jwalker] from comment #2)
> Comment on attachment 832939 [details] [diff] [review]
> color-names-out-of-context-939098.patch
> 
> Review of attachment 832939 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When would we want onlyParseColorsAfterSupportingCSSProperty=false ?

When the value string doesn't contain a property name e.g. from CSS tools. It needs to be true to correctly parse html attributes in the markup view.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> (In reply to Joe Walker [:jwalker] from comment #2)
> > Comment on attachment 832939 [details] [diff] [review]
> > color-names-out-of-context-939098.patch
> > 
> > Review of attachment 832939 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > When would we want onlyParseColorsAfterSupportingCSSProperty=false ?
> 
> When the value string doesn't contain a property name e.g. from CSS tools.
> It needs to be true to correctly parse html attributes in the markup view.

So the name should reflect that difference then?
(In reply to Joe Walker [:jwalker] from comment #5)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #3)
> > (In reply to Joe Walker [:jwalker] from comment #2)
> > > Comment on attachment 832939 [details] [diff] [review]
> > > color-names-out-of-context-939098.patch
> > > 
> > > Review of attachment 832939 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > When would we want onlyParseColorsAfterSupportingCSSProperty=false ?
> > 
> > When the value string doesn't contain a property name e.g. from CSS tools.
> > It needs to be true to correctly parse html attributes in the markup view.
> 
> So the name should reflect that difference then?

I have renamed it to isHTMLAttribute and reversed the logic. I have also added a descriptive comment.
Attachment #832939 - Attachment is obsolete: true
Attachment #832939 - Flags: review?(jwalker)
Attachment #832967 - Flags: review?(jwalker)
Comment on attachment 832967 [details] [diff] [review]
color-names-out-of-context-939098.patch

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

::: toolkit/devtools/output-parser.js
@@ +193,2 @@
>          }
>        }

Isn't this the same code twice?
(In reply to Joe Walker [:jwalker] from comment #7)
> Comment on attachment 832967 [details] [diff] [review]
> color-names-out-of-context-939098.patch
> 
> Review of attachment 832967 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/output-parser.js
> @@ +193,2 @@
> >          }
> >        }
> 
> Isn't this the same code twice?

Yes it was.

Working around this has meant making a couple of changes to the parser itself as I don't like having more than one local function if it can be avoided:
- Replaced dirty flag with break and continue to enhance performance.
- Moved max iteration check to start of loop so that it is also checked.
- Simplified max iteration check.
- Moved local functions to OutputParser Prototype. Admittedly, _trimMatchFromStart is now a one liner but it does make the parser easier to read.

I have created a try run but all tests pass locally.

Try:
https://tbpl.mozilla.org/?tree=Try&rev=f27cbfc10758
Attachment #832967 - Attachment is obsolete: true
Attachment #832967 - Flags: review?(jwalker)
Attachment #8333881 - Flags: review?(jwalker)
Duplicate of this bug: 940096
Attachment #8333881 - Flags: review?(jwalker) → review+
https://hg.mozilla.org/integration/fx-team/rev/4095400970fd
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4095400970fd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Comment on attachment 8333881 [details] [diff] [review]
Removed dirty flag, moved local functions to prototype

[Approval Request Comment]
Bug caused by (feature/regressing bug #): DevTools output parser
User impact if declined: Class names in markup panel could be replaced by color swatches.
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None
Attachment #8333881 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 945229
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #12)
> Comment on attachment 8333881 [details] [diff] [review]
> Removed dirty flag, moved local functions to prototype
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): DevTools output parser
> User impact if declined: Class names in markup panel could be replaced by
> color swatches.
> Testing completed (on m-c, etc.): Yes
> Risk to taking this patch (and alternatives if risky): Low
> String or IDL/UUID changes made by this patch: None

what is the blocking/regressing bug here ?
(In reply to bhavana bajaj [:bajaj] from comment #14)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #12)
> > Comment on attachment 8333881 [details] [diff] [review]
> > Removed dirty flag, moved local functions to prototype
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #): DevTools output parser
> > User impact if declined: Class names in markup panel could be replaced by
> > color swatches.
> > Testing completed (on m-c, etc.): Yes
> > Risk to taking this patch (and alternatives if risky): Low
> > String or IDL/UUID changes made by this patch: None
> 
> what is the blocking/regressing bug here ?

This bug was introduced when the output parser was created in bug 918716.

Until this is fixed in Aurora class names in markup panel may be replaced by color swatches e.g. class="red" would be displayed as class="#F00", class="inherit" would be displayed as class="black", CSS shortcut properties such as border: 1px solid red will not receive color swatches plus various other issues.
Attachment #8333881 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
There was a lot of rebasing needed to get this to apply on Aurora so I have created a try run:
https://tbpl.mozilla.org/?tree=Try&rev=2ddb2ec6837b
Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.