Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2)

Issue 5765003219042304: EEH inspector integration fix (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 7 months ago by saroyanm
Modified:
5 years, 7 months ago
Reviewers:
Wladimir Palant
Visibility:
Public.

Description

This issue is related to current ticket: https://issues.adblockplus.org/ticket/317

Patch Set 1 #

Total comments: 3

Patch Set 2 : Fix for light DevTools theme #

Total comments: 8

Patch Set 3 : Nits fixes #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -6 lines) Patch
A chrome/skin/devToolsOverlay.css View 1 2 1 chunk +14 lines, -0 lines 1 comment Download
M lib/inspectorObserver.js View 1 2 2 chunks +8 lines, -6 lines 0 comments Download

Messages

Total messages: 6
saroyanm
Wladimir please have a look when you will have time. Thanks in advance. http://codereview.adblockplus.org/5765003219042304/diff/5629499534213120/lib/inspectorObserver.js File ...
5 years, 7 months ago (2014-04-14 15:38:01 UTC) #1
Wladimir Palant
http://codereview.adblockplus.org/5765003219042304/diff/5629499534213120/lib/inspectorObserver.js File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/5765003219042304/diff/5629499534213120/lib/inspectorObserver.js#newcode53 lib/inspectorObserver.js:53: button.setAttribute("class", "devtools-toolbarbutton"); On 2014/04/14 15:38:01, saroyanm wrote: > Not ...
5 years, 7 months ago (2014-04-14 21:40:39 UTC) #2
saroyanm
Updated, hope this will work. http://codereview.adblockplus.org/5765003219042304/diff/5629499534213120/lib/inspectorObserver.js File lib/inspectorObserver.js (right): http://codereview.adblockplus.org/5765003219042304/diff/5629499534213120/lib/inspectorObserver.js#newcode53 lib/inspectorObserver.js:53: button.setAttribute("class", "devtools-toolbarbutton"); On 2014/04/14 ...
5 years, 7 months ago (2014-04-15 10:21:49 UTC) #3
Wladimir Palant
http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/chrome/skin/devToolsOverlay.css File chrome/skin/devToolsOverlay.css (right): http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/chrome/skin/devToolsOverlay.css#newcode9 chrome/skin/devToolsOverlay.css:9: #inspector-abp-elemhide-toolbutton>image Nit: please add a space before and after ...
5 years, 7 months ago (2014-04-15 13:38:19 UTC) #4
saroyanm
Thanks for notes Wladimi, Please look at updated patch. http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/chrome/skin/devToolsOverlay.css File chrome/skin/devToolsOverlay.css (right): http://codereview.adblockplus.org/5765003219042304/diff/5122315436163072/chrome/skin/devToolsOverlay.css#newcode9 chrome/skin/devToolsOverlay.css:9: ...
5 years, 7 months ago (2014-04-15 13:54:42 UTC) #5
Wladimir Palant
5 years, 7 months ago (2014-04-15 14:03:35 UTC) #6
LGTM with the remaining nit fixed.

http://codereview.adblockplus.org/5765003219042304/diff/5194384987389952/chro...
File chrome/skin/devToolsOverlay.css (right):

http://codereview.adblockplus.org/5765003219042304/diff/5194384987389952/chro...
chrome/skin/devToolsOverlay.css:13: -moz-image-region: rect(0px, 16px, 16px,
0px);
We should not go down into the anonymous elements unless necessary -
list-style-image and -moz-image-region should still be applied to
#ehh-inspector-toolbarbutton itself. filter property is the only exception,
there we cannot do it better.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5