Closed Bug 1289425 Opened 8 years ago Closed 8 years ago

replace uses of getCSSPropertyNames

Categories

(DevTools :: Framework, enhancement, P1)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox51 fixed)

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

People

(Reporter: tromey, Assigned: gregtatum)

References

Details

(Whiteboard: [reserve-html])

Attachments

(4 files)

Bug 1289425 - Add a getClientCssProperties method to the CssProperties;
58 bytes, text/x-review-board-request
tromey
: review+
Details
Bug 1289425 - Remove domUtils from the InplaceEditor;
58 bytes, text/x-review-board-request
tromey
: review+
Details
Bug 1289425 - Remove domUtils from source editor and autocomplete;
58 bytes, text/x-review-board-request
tromey
: review+
Details
Bug 1289425 - Allow sourceeditor to fallback to client-side css properties;
58 bytes, text/x-review-board-request
tromey
: review+
Details
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.
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: