Closed Bug 1235781 Opened 6 years ago Closed 6 years ago

Remove preprocessing from common.css

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(firefox46 fixed)

RESOLVED FIXED
Firefox 46
Tracking Status
firefox46 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(2 files, 4 obsolete files)

No description provided.
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #8702885 - Flags: review?(bgrinstead)
Attached patch patch v2Splinter Review
Added missing splitters.css...
Attachment #8703660 - Flags: review?(bgrinstead)
Attachment #8702885 - Attachment is obsolete: true
Attachment #8702885 - Flags: review?(bgrinstead)
Assignee: nobody → poirot.alex
Comment on attachment 8703660 [details] [diff] [review]
patch v2

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

Looks like a nice refactor, thanks!

::: browser/base/content/browser.xul
@@ +7,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  <?xml-stylesheet href="chrome://browser/content/browser.css" type="text/css"?>
>  <?xml-stylesheet href="chrome://browser/content/places/places.css" type="text/css"?>
> +<?xml-stylesheet href="chrome://devtools/skin/browser.css" type="text/css"?>

What do you think about renaming this to something like devtools-browser.css?  Concerned we might be in the way of the rest of the frontend team by reusing the browser.css name.

::: devtools/client/themes/browser.css
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +@import url("splitters.css");

Hopefully this won't cause any talos regressions due to loading another CSS file.  If so I guess we could have some code that only loads these styles if/when devtools is opened.
Attachment #8703660 - Flags: review?(bgrinstead) → review+
Duplicate of this bug: 957663
Backed out because it broke eyedropper test in dt tests:

https://treeherder.mozilla.org/logviewer.html#?job_id=6514176&repo=fx-team

08:31:12     INFO -  70 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | swatch didn't change after pressing ESC -
08:31:12     INFO -  71 INFO Console message: [JavaScript Warning: "Property contained reference to invalid variable.  Error in parsing value for 'font-family'.  Falling back to 'inherit'." {file: "chrome://devtools/skin/common.css" line: 24 column: 505 source: " var(--monospace-font-family)"}]
08:31:12     INFO -  72 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | dropper opened -
08:31:12     INFO -  73 INFO Console message: [JavaScript Warning: "Property contained reference to invalid variable.  Error in parsing value for 'font-family'.  Falling back to 'inherit'." {file: "chrome://devtools/skin/common.css" line: 24 column: 505 source: " var(--monospace-font-family)"}]
08:31:12     INFO -  74 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | swatch changed colors - Got rgb(228, 239, 250), expected rgb(255, 255, 85)
08:31:12     INFO -  Stack trace:
08:31:12     INFO -      chrome://mochikit/content/browser-test.js:test_is:979
08:31:12     INFO -      chrome://mochitests/content/browser/devtools/client/inspector/rules/test/browser_rules_eyedropper.js:testSelect:104
08:31:12     INFO -      Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:933:23
08:31:12     INFO -      this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7
08:31:12     INFO -      this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:11
08:31:12     INFO -      promise callback*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:9
08:31:12     INFO -      this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:775:7
08:31:12     INFO -      this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:710:7
08:31:12     INFO -      openEyedropper/</<@chrome://mochitests/content/browser/devtools/client/inspector/rules/test/browser_rules_eyedropper.js:142:7
08:31:12     INFO -      EventEmitter_once/handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:92:9
08:31:12     INFO -      EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:147:11
08:31:12     INFO -      SwatchColorPickerTooltip.prototype<._openEyeDropper@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/Tooltip.js:1217:5
08:31:12     INFO -      openEyedropper/<@chrome://mochitests/content/browser/devtools/client/inspector/rules/test/browser_rules_eyedropper.js:144:5
08:31:12     INFO -      EventEmitter_once/handler@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:92:9
08:31:12     INFO -      EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/event-emitter.js:147:11
08:31:12     INFO -      @resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/Tooltip.js:199:11
08:31:12     INFO -      EventListener.handleEvent*Tooltip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/Tooltip.js:203:5
08:31:12     INFO -      SwatchBasedEditorTooltip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/Tooltip.js:957:18
08:31:12     INFO -      SwatchColorPickerTooltip@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/shared/widgets/Tooltip.js:1128:3
08:31:12     INFO -      TooltipsOverlay.prototype.addToView@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/styleinspector/style-inspector-overlays.js:281:26
08:31:12     INFO -      CssRuleView@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/rules/rules.js:570:3
08:31:12     INFO -      RuleViewTool@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/styleinspector/style-inspector.js:31:15
08:31:12     INFO -      window.setPanel@chrome://devtools/content/inspector/rules/rules.xhtml:27:25
08:31:12     INFO -      ToolSidebar.prototype.addTab/onIFrameLoaded@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/sidebar.js:244:9
08:31:12     INFO -      EventListener.handleEvent*ToolSidebar.prototype.addTab@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/sidebar.js:249:5
08:31:12     INFO -      InspectorPanel.prototype.setupSidebar@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:355:5
08:31:12     INFO -      InspectorPanel.prototype._deferredOpen@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:212:5
08:31:12     INFO -      InspectorPanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:109:14
08:31:12     INFO -      Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:933:23
08:31:12     INFO -      this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7
08:31:12     INFO -      Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:743:11
08:31:12     INFO -      this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:775:7
08:31:12     INFO -      this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:710:7
08:31:12     INFO -      Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9
08:31:12     INFO -      DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
08:31:12     INFO -      frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
08:31:12     INFO -      exports.WalkerFront<.querySelector<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:3359:12
08:31:12     INFO -      InspectorPanel.prototype._getDefaultNodeForSelection/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:265:14
08:31:12     INFO -      Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:933:23
08:31:12     INFO -      this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7
08:31:12     INFO -      Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:743:11
08:31:12     INFO -      this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:775:7
08:31:12     INFO -      this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:710:7
08:31:12     INFO -      Front<.onPacket/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1236:9
08:31:12     INFO -      DevTools RDP*Front<.request@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1178:7
08:31:12     INFO -      frontProto/</proto[name]@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/protocol.js:1338:14
08:31:12     INFO -      exports.InspectorFront<.getPageStyle<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/inspector.js:3932:12
08:31:12     INFO -      InspectorPanel.prototype._getPageStyle@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:226:12
08:31:12     INFO -      InspectorPanel.prototype.open/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:105:14
08:31:12     INFO -      Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:933:23
08:31:12     INFO -      this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:812:7
08:31:12     INFO -      Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:743:11
08:31:12     INFO -      this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:775:7
08:31:12     INFO -      Promise.prototype.then@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:450:5
08:31:12     INFO -      InspectorPanel.prototype.open@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/inspector/inspector-panel.js:104:12
08:31:12     INFO -      Toolbox.prototype.loadTool/onLoad@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:1247:19
08:31:12     INFO -  75 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | div's color set to body color after dropper -
08:31:12     INFO -  76 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | eyedropper telemetry value correct for DEVTOOLS_PICKER_EYEDROPPER_OPENED_BOOLEAN -
08:31:12     INFO -  77 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_eyedropper.js | eyedropper telemetry value correct for DEVTOOLS_PICKER_EYEDROPPER_OPENED_PER_USER_FLAG -
08:31:12     INFO -  78 INFO Leaving test
Flags: needinfo?(poirot.alex)
I need these additional tweaks to fix the eyedropper...
That's because we also pull common.css from this xul document
but we don't use theme-switching in it. So that platform attribute isn't set.

Also, .devtools-eyedropper-panel has to be in browser.xul scope
as we insert the xul:panel there and not in toolbox documents.

With that, I get a greener try:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=97e282fdb5ea&selectedJob=15293383
Attachment #8706604 - Flags: review?(bgrinstead)
Attached image eye-dropper-color.png (obsolete) —
I see a white 'border' around the eyedropper on osx with these patches applied
Comment on attachment 8706630 [details]
eye-dropper-color.png

Nevermind, looks like ./mach build faster wasn't working..  With the updated patch it looks as expected
Attachment #8706630 - Attachment is obsolete: true
Comment on attachment 8706604 [details] [diff] [review]
Fix per platform css and browser css rule for eyedropper.

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

This works for me.  I'd prefer to not have copy/pasted code for the 'platform' bit though

::: devtools/client/eyedropper/eyedropper.js
@@ +369,5 @@
>      else if (this.format == "hsl") {
>        valueBox.style.width = HSL_BOX_WIDTH + "px";
>      }
>  
> +    let os;

What about loading theme-switching.js into the eyedropper.xul panel?  Then we wouldn't need to copy/paste this logic.  To save from unnecessary css loading we could add a new attribute on the root element like "no-theme" that'd be similar to "force-theme" but prevented theme-switching from actually loading a file (instead it'd just set the platform attribute).  I prefer that theme-switching is loaded in every frame for exactly this reason.

Alternatively what about using shared/system -> getSystemInfo().os (or adding a new constant on that object that always returns "win"/"mac"/"linux") and using it in both places?

::: devtools/client/eyedropper/eyedropper.xul
@@ +37,5 @@
>        <label id="color-value" class="devtools-monospace">
>        </label>
>      </hbox>
>    </hbox>
> +</window>

What's this change?
Attachment #8706604 - Flags: review?(bgrinstead) → feedback+
(In reply to Brian Grinstead [:bgrins] from comment #16)
> ::: devtools/client/eyedropper/eyedropper.js
> @@ +369,5 @@
> >      else if (this.format == "hsl") {
> >        valueBox.style.width = HSL_BOX_WIDTH + "px";
> >      }
> >  
> > +    let os;
> 
> What about loading theme-switching.js into the eyedropper.xul panel?

I did that with the "no-theme" attribute.

> ::: devtools/client/eyedropper/eyedropper.xul
> @@ +37,5 @@
> >        <label id="color-value" class="devtools-monospace">
> >        </label>
> >      </hbox>
> >    </hbox>
> > +</window>
> 
> What's this change?

EOF space thing git does automagically. I removed it.
Attachment #8707009 - Flags: review?(bgrinstead)
Attachment #8706604 - Attachment is obsolete: true
Comment on attachment 8707009 [details] [diff] [review]
Fix per platform css and browser css rule for eyedropper - v2

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

Thanks!
Attachment #8707009 - Flags: review?(bgrinstead) → review+
Attachment #8707009 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5292f5305f46
https://hg.mozilla.org/mozilla-central/rev/8cd5149485cb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Flags: needinfo?(poirot.alex)
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	        beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	        beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.