Closed Bug 1289425 Opened 4 years ago Closed 4 years ago

replace uses of getCSSPropertyNames

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Iteration:
51.3 - Sep 19
Tracking Status
firefox51 --- fixed

People

(Reporter: tromey, Assigned: gregtatum)

References

Details

(Whiteboard: [reserve-html])

Attachments

(4 files)

Uses of getCSSPropertyNames must be replaced for the de-chrome-ification project.
Whiteboard: [devtools-html] → [devtools-html] [triage]
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
Whiteboard: [devtools-html] → [reserve-html]
Assignee: nobody → gtatum
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
Blocks: 1292250
This creates an easy interface for getting the static css database, and
splits up the initCssProperties function into smaller re-usable parts.

Review commit: https://reviewboard.mozilla.org/r/69736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69736/
Attachment #8778437 - Flags: review?(ttromey)
Attachment #8778438 - Flags: review?(ttromey)
Attachment #8778439 - Flags: review?(ttromey)
Comment on attachment 8778437 [details]
Bug 1289425 - Add a getClientCssProperties method to the CssProperties;

https://reviewboard.mozilla.org/r/69736/#review67178

Thank you for doing this.  Looks good.  I have one small suggestion but it's up to you.

::: devtools/shared/fronts/css-properties.js:175
(Diff revision 1)
>  
>  /**
> + * Get a client-side CssProperties. This is useful for dependencies in tests.
> + * @return {CssProperties}
> + */
> +function getClientCssProperties() {

If this is only for tests, then I think it would be good to stick something like "ForTesting" in the name.
Attachment #8778437 - Flags: review?(ttromey) → review+
Comment on attachment 8778438 [details]
Bug 1289425 - Remove domUtils from the InplaceEditor;

https://reviewboard.mozilla.org/r/69738/#review67180

Thank you.  Looks great.
Attachment #8778438 - Flags: review?(ttromey) → review+
Comment on attachment 8778439 [details]
Bug 1289425 - Remove domUtils from source editor and autocomplete;

https://reviewboard.mozilla.org/r/69740/#review67184

Thank you.
This looks good, though I'd like to understand the vim_test.js change.

::: devtools/client/sourceeditor/editor.js:313
(Diff revision 1)
> -      scssSpec.colorKeywords = cssColors;
> -      scssSpec.valueKeywords = cssValues;
> +        scssSpec.colorKeywords = colorKeywords;
> +        scssSpec.valueKeywords = valueKeywords;
> -      win.CodeMirror.defineMIME("text/x-scss", scssSpec);
> +        win.CodeMirror.defineMIME("text/x-scss", scssSpec);
>  
> -      win.CodeMirror.commands.save = () => this.emit("saveRequested");
> +        win.CodeMirror.commands.save = () => this.emit("saveRequested");
> +      } else if(this.config.mode === Editor.modes.css) {

Space after the "if".

::: devtools/client/sourceeditor/test/codemirror/vim_test.js:3143
(Diff revision 1)
>    cm.setSize(600, 35*cm.defaultTextHeight());
>    cm.setCursor(lineNum, 0);
>    helpers.doKeys('z', 'b');
>    var scrollInfo = cm.getScrollInfo();
> -  eq(scrollInfo.top + scrollInfo.clientHeight, cm.charCoords(Pos(lineNum, 0), 'local').bottom);
> +  eq(scrollInfo.top + scrollInfo.clientHeight,
> +     Math.floor(cm.charCoords(Pos(lineNum, 0), 'local').bottom));

I don't understand how this change relates to the rest of the patch.
Attachment #8778439 - Flags: review?(ttromey) → review+
Comment on attachment 8778439 [details]
Bug 1289425 - Remove domUtils from source editor and autocomplete;

https://reviewboard.mozilla.org/r/69740/#review67184

> Space after the "if".

Thanks, didn't realize this was eslint ignored.

> I don't understand how this change relates to the rest of the patch.

Ugh. Yeah this value is 0.5 off. I have no idea how this would change, so is probably something I should look at further. I'm pretty sure it's a high pixel density display problem with my Macbook's retina screen. Maybe something subtle shifted with my code changes. I'll check it out again.
Comment on attachment 8778439 [details]
Bug 1289425 - Remove domUtils from source editor and autocomplete;

https://reviewboard.mozilla.org/r/69740/#review67184

> Ugh. Yeah this value is 0.5 off. I have no idea how this would change, so is probably something I should look at further. I'm pretty sure it's a high pixel density display problem with my Macbook's retina screen. Maybe something subtle shifted with my code changes. I'll check it out again.

I re-ran it on an external monitor without my changes and it worked fine.
Duplicate of this bug: 1292250
Comment on attachment 8780673 [details]
Bug 1289425 - Allow sourceeditor to fallback to client-side css properties;

https://reviewboard.mozilla.org/r/71320/#review68850

Thanks for doing this.
It looks reasonable to me.  I have one nit, nothing major.

::: devtools/client/sourceeditor/editor.js:242
(Diff revision 1)
>    if (this.config.cssProperties) {
>      this.config.autocompleteOpts.cssProperties = this.config.cssProperties;
>    }
>  
> +  // Use a static client-side database of CSS values if none is provided.
> +  if (!this.config.cssProperties) {

I think this can be merged with the previous "if" to make the overall effect a bit cleaner.
Attachment #8780673 - Flags: review?(ttromey) → review+
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
Blocks: 1292592
I just pushed a fix for that eslint error in the above test run.
Iteration: 51.2 - Aug 29 → 51.3 - Sep 12
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9f0e8136f7fe
Add a getClientCssProperties method to the CssProperties; r=tromey
https://hg.mozilla.org/integration/autoland/rev/39d8855f58a9
Remove domUtils from the InplaceEditor; r=tromey
https://hg.mozilla.org/integration/autoland/rev/3741c12fb254
Remove domUtils from source editor and autocomplete; r=tromey
https://hg.mozilla.org/integration/autoland/rev/6bc76fe903e1
Allow sourceeditor to fallback to client-side css properties; r=tromey
Keywords: checkin-needed
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.