939098 - Output parser parses color names out of context in markup panel

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Is converted to:
class="someclass #6363CE someotherclass #F00"

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: color-names-out-of-context-939098.patch

Comment 2

[Joe Walker [:jwalker]]

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 ?

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(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.

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Obligatory try run:
https://tbpl.mozilla.org/?tree=Try&rev=08666cd7a005

Comment 5

[Joe Walker [:jwalker]]

(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?

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: color-names-out-of-context-939098.patch

(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.

Comment 7

[Joe Walker [: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?

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Removed dirty flag, moved local functions to prototype

(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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4095400970fd

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 14

[bhavana bajaj [:bajaj]]

(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 ?

Comment 15

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(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.

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f3d6a773ab72