Issue 5646124035604480: Issue 154 - Added UI for devtools panel on Chrome (Closed)
|
Created:
Jan. 7, 2015, 1:36 p.m. by Sebastian Noack Modified:
Jan. 31, 2016, 12:58 p.m. Reviewers:
Thomas Greiner CC:
kzar, Wladimir Palant Visibility:
Public. |
DescriptionIssue 154 - Added UI for devtools panel on Chrome
Patch Set 1 #
Total comments: 54
Patch Set 2 : Addressed some comments #Patch Set 3 : When using <!DOCTYPE html> I have to set "height:100%" for the <html> element #Patch Set 4 : Addressed Sven's comments and use an HTML template for table rows #Patch Set 5 : Addressed comments #Patch Set 6 : Fixed bugs #Patch Set 7 : Rebased and adapted for API changes #
Total comments: 12
Patch Set 8 : Addressed comments #Patch Set 9 : Rebased, enabled strict mode, updated copyright year #Patch Set 10 : Added new request types #
MessagesTotal messages: 13
I only skimmed the implementation, mostly reviewed the proposed abstraction layer changes. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... File ext/devtools.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... ext/devtools.js:25: addListener: function(listener) Quite frankly, I don't like the idea of having a different API here - I think that the Chrome abstraction layer (ext/content.js in particular) can handle this implicitly rather than increasing the complexity for all implementations. Seeing that chrome.runtime.connect is available to content scripts as well, it should probably connect unconditionally and send all messages through the connection. The background page can maintain a list of ports mapped by tab ID so that sending *to* the content script will also succeed. This will also have the advantage that using webNavigation won't be necessary any more, Port.onDisconnect() will do. This will solve the same issue for the popup as well - it isn't specific to the devtools. Shouldn't be too complicated? http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... ext/devtools.js:99: }); I'd rather have proper message exchange simulated, particularly simulating the "requests.listen" message as well. The corresponding code should really be in background.js where we have the "application logic" - when receiving "requests.listen" it should send these responses immediately and maybe also send another response in a timed interval.
http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... File ext/devtools.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... ext/devtools.js:25: addListener: function(listener) On 2015/01/10 08:33:29, Wladimir Palant wrote: > Quite frankly, I don't like the idea of having a different API here - I think > that the Chrome abstraction layer (ext/content.js in particular) can handle this > implicitly rather than increasing the complexity for all implementations. Seeing > that chrome.runtime.connect is available to content scripts as well, it should > probably connect unconditionally and send all messages through the connection. > The background page can maintain a list of ports mapped by tab ID so that > sending *to* the content script will also succeed. This will also have the > advantage that using webNavigation won't be necessary any more, > Port.onDisconnect() will do. This will solve the same issue for the popup as > well - it isn't specific to the devtools. Shouldn't be too complicated? Yes, it will be fairly complicated. Ports are very low-level, and doesn't support sending responses to individual messages. So we would need to implement response handling like we did on Safari also for Chrome. And even then devtools panels aren't tabs. So the abstraction layer as it is right now can't simply handle it (without extending the API) anyway. With the devtools panel we don't need to bother about responses, and using ports is rather convenient here, beside its the only option anyway. Yes, we are going to have a similar issue with the popup when switching to async messages, but this is a separate issue. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... ext/devtools.js:99: }); On 2015/01/10 08:33:29, Wladimir Palant wrote: > I'd rather have proper message exchange simulated, particularly simulating the > "requests.listen" message as well. The corresponding code should really be in > background.js where we have the "application logic" - when receiving > "requests.listen" it should send these responses immediately and maybe also send > another response in a timed interval. This would makes things significantly more complex. And I don't see any advantage. This is just that you can run the UI standalone. Nobody except ourselves will ever do that. So I don't think it's worth making thing unnecessary complicated here.
http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... File devtools-panel.html (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:1: <html> License header and document type are missing http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:4: <script type="application/javascript" src="ext/common.js"></script> Declaring the type attribute is no longer necessary http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:9: <div id="header"> What about using <header> here instead? http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:9: <div id="header"> The structure is a bit scattered so instead of header - table (contains selects as well as table headers) - tr - td div#items - table - tr - td footer I'd suggest to merge the two tables into a single one header (contains selects) table - thead (contains table headers) - tr - th - tbody#items - tr - td footer http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:30: <option>OTHER</option> I noticed that we also offer the following content types: - OBJECT_SUBREQUEST - MEDIA - FONT Any reason why not to include them? http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:55: <div id="footer"> Same here with the <footer> tag http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... File devtools-panel.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:48: function createRecord(request, filter, port) Seems like HTML templates is what you'd want to use here which should be supported by Chrome 28. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:62: urlElement.innerHTML = " "; I assume you do this to ensure a minimum height for this element. If so you should rather use the `min-height` CSS property. However, why would there be no URL in the first place? http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:96: if (request.url) Which requests don't have URLs associated to them? http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:100: if ("openResource" in ext.devtools.panels && request.type != "POPUP") Why would `ext.devtools.panels.openResource` not be available? http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:103: urlElement.addEventListener("click", function() No need to add an event listener to each row individually. You can catch the event higher up in the hierarchy. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... File ext/devtools.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... ext/devtools.js:25: addListener: function(listener) On 2015/01/10 08:53:45, Sebastian Noack wrote: > On 2015/01/10 08:33:29, Wladimir Palant wrote: > > Quite frankly, I don't like the idea of having a different API here - I think > > that the Chrome abstraction layer (ext/content.js in particular) can handle > this > > implicitly rather than increasing the complexity for all implementations. > Seeing > > that chrome.runtime.connect is available to content scripts as well, it should > > probably connect unconditionally and send all messages through the connection. > > The background page can maintain a list of ports mapped by tab ID so that > > sending *to* the content script will also succeed. This will also have the > > advantage that using webNavigation won't be necessary any more, > > Port.onDisconnect() will do. This will solve the same issue for the popup as > > well - it isn't specific to the devtools. Shouldn't be too complicated? > > Yes, it will be fairly complicated. Ports are very low-level, and doesn't > support sending responses to individual messages. So we would need to implement > response handling like we did on Safari also for Chrome. And even then devtools > panels aren't tabs. So the abstraction layer as it is right now can't simply > handle it (without extending the API) anyway. > > With the devtools panel we don't need to bother about responses, and using ports > is rather convenient here, beside its the only option anyway. Yes, we are going > to have a similar issue with the popup when switching to async messages, but > this is a separate issue. Right, the existing abstraction layer would probably be insufficient for this case. However, I do think we should look into performance differences between regular message passing and using ports because we're sending quite a bunch of messages so it might make sense for us to use ports. That's outside of the scope of this review, of course. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... ext/devtools.js:99: }); On 2015/01/10 08:53:45, Sebastian Noack wrote: > On 2015/01/10 08:33:29, Wladimir Palant wrote: > > I'd rather have proper message exchange simulated, particularly simulating the > > "requests.listen" message as well. The corresponding code should really be in > > background.js where we have the "application logic" - when receiving > > "requests.listen" it should send these responses immediately and maybe also > send > > another response in a timed interval. > > This would makes things significantly more complex. And I don't see any > advantage. This is just that you can run the UI standalone. Nobody except > ourselves will ever do that. So I don't think it's worth making thing > unnecessary complicated here. More complex because we're using two different methods for passing messages or are there other issues you're thinking of? http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... File skin/devtools-panel.css (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... skin/devtools-panel.css:3: font-family: 'Lucida Grande', 'Segoe UI', Tahoma, Arial, sans-serif; In the Networks panel they're using the following CSS for font-family. Therefore please insert "Ubuntu" to the list before "Tahoma". body { ... font-family: Lucida Grande, sans-serif; color: #222; ... } body.platform-linux { color: rgb(48, 57, 66); font-family: Ubuntu, Arial, sans-serif; } body.platform-mac { color: rgb(48, 57, 66); font-family: 'Lucida Grande', sans-serif; } body.platform-windows { font-family: 'Segoe UI', Tahoma, sans-serif; } http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... skin/devtools-panel.css:12: display: flex; According to http://caniuse.com/#search=flex Chrome 28 still requires the "-webkit" prefix for flexbox properties/values. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... skin/devtools-panel.css:50: * with the table below. But we don't want the scrollbar to be visible there. I guess that would be redundant if you merge the two tables into one, right? http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... skin/devtools-panel.css:133: text-decoration: underline; It'd be great to make the entire cell clickable and not just the link (as it is in the Networks panel).
http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... File devtools-panel.html (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:1: <html> On 2015/01/12 10:37:54, Thomas Greiner wrote: > License header You are right. I wasn't aware that we add the GPL header also to HTML files. > and document type are missing It's optional in HTML5. And I personally prefer to omit. But it seems that we usually still specify the doctype declaration. So in favor of consistence, I added it. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:4: <script type="application/javascript" src="ext/common.js"></script> On 2015/01/12 10:37:54, Thomas Greiner wrote: > Declaring the type attribute is no longer necessary Done. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:9: <div id="header"> On 2015/01/12 10:37:54, Thomas Greiner wrote: > What about using <header> here instead? Done. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:9: <div id="header"> On 2015/01/12 10:37:54, Thomas Greiner wrote: > The structure is a bit scattered so instead of > > header > - table (contains selects as well as table headers) > - tr > - td > div#items > - table > - tr > - td > footer > > I'd suggest to merge the two tables into a single one > > header (contains selects) > table > - thead (contains table headers) > - tr > - th > - tbody#items > - tr > - td > footer Initially I used that structure. But flexbox requires an additional container around the the table header and content area. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:30: <option>OTHER</option> On 2015/01/12 10:37:54, Thomas Greiner wrote: > I noticed that we also offer the following content types: > > - OBJECT_SUBREQUEST > - MEDIA > - FONT > > Any reason why not to include them? We don't have those types on Chrome/Opera/Safari. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:55: <div id="footer"> On 2015/01/12 10:37:54, Thomas Greiner wrote: > Same here with the <footer> tag Done. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... File devtools-panel.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:48: function createRecord(request, filter, port) On 2015/01/12 10:37:54, Thomas Greiner wrote: > Seems like HTML templates is what you'd want to use here which should be > supported by Chrome 28. Sounds good. I will look into it. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:62: urlElement.innerHTML = " "; On 2015/01/12 10:37:54, Thomas Greiner wrote: > I assume you do this to ensure a minimum height for this element. If so you > should rather use the `min-height` CSS property. Doesn't seem to do the trick when using flexbox. > However, why would there be no URL in the first place? See below. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:96: if (request.url) On 2015/01/12 10:37:54, Thomas Greiner wrote: > Which requests don't have URLs associated to them? Element hiding filter matches. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:100: if ("openResource" in ext.devtools.panels && request.type != "POPUP") On 2015/01/12 10:37:54, Thomas Greiner wrote: > Why would `ext.devtools.panels.openResource` not be available? It's new in Chrome 38. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:103: urlElement.addEventListener("click", function() On 2015/01/12 10:37:54, Thomas Greiner wrote: > No need to add an event listener to each row individually. You can catch the > event higher up in the hierarchy. I know. But then I'd need to double check whether the element clicked on is an action button, and extract the URL from the DOM. It's much simpler this way. And I don't care that too many event listeners slow down the page in IE6. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... File ext/devtools.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... ext/devtools.js:99: }); On 2015/01/12 10:37:54, Thomas Greiner wrote: > On 2015/01/10 08:53:45, Sebastian Noack wrote: > > On 2015/01/10 08:33:29, Wladimir Palant wrote: > > > I'd rather have proper message exchange simulated, particularly simulating > the > > > "requests.listen" message as well. The corresponding code should really be > in > > > background.js where we have the "application logic" - when receiving > > > "requests.listen" it should send these responses immediately and maybe also > > send > > > another response in a timed interval. > > > > This would makes things significantly more complex. And I don't see any > > advantage. This is just that you can run the UI standalone. Nobody except > > ourselves will ever do that. So I don't think it's worth making thing > > unnecessary complicated here. > > More complex because we're using two different methods for passing messages or > are there other issues you're thinking of? What Wladimir is suggesting is to inject an iframe running a simulated background page, passing messages with postMessage. We have to do this for the first run page anyway. But since we can't do regular messaging, but have to use ports here, this would make things more complicated, for no benefit whatsoever. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... File skin/devtools-panel.css (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... skin/devtools-panel.css:3: font-family: 'Lucida Grande', 'Segoe UI', Tahoma, Arial, sans-serif; On 2015/01/12 10:37:54, Thomas Greiner wrote: > In the Networks panel they're using the following CSS for font-family. Therefore > please insert "Ubuntu" to the list before "Tahoma". > > body { > ... > font-family: Lucida Grande, sans-serif; > color: #222; > ... > } > > body.platform-linux { > color: rgb(48, 57, 66); > font-family: Ubuntu, Arial, sans-serif; > } > > body.platform-mac { > color: rgb(48, 57, 66); > font-family: 'Lucida Grande', sans-serif; > } > > body.platform-windows { > font-family: 'Segoe UI', Tahoma, sans-serif; > } Done. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... skin/devtools-panel.css:12: display: flex; On 2015/01/12 10:37:54, Thomas Greiner wrote: > According to http://caniuse.com/#search=flex Chrome 28 still requires the > "-webkit" prefix for flexbox properties/values. Yep, we are dropping support for Chrome 28 (see the other review). According to Wladimir only 0.06% are using Chrome 28. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... skin/devtools-panel.css:50: * with the table below. But we don't want the scrollbar to be visible there. On 2015/01/12 10:37:54, Thomas Greiner wrote: > I guess that would be redundant if you merge the two tables into one, right? As outlined in the other file I can't merge the tables. Even if you ignore the flexbox issue mentioned there, it wouldn't be possible to have a fixed header aymore, when merging the tables. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... skin/devtools-panel.css:133: text-decoration: underline; On 2015/01/12 10:37:54, Thomas Greiner wrote: > It'd be great to make the entire cell clickable and not just the link (as it is > in the Networks panel). I disagree. Making the document domain clickable would be confusing, since some users might expect to get taken to the parent document.
http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... File ext/devtools.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... ext/devtools.js:25: addListener: function(listener) On 2015/01/10 08:53:45, Sebastian Noack wrote: > Yes, it will be fairly complicated. Ports are very low-level, and doesn't > support sending responses to individual messages. I see, that's making things too complicated indeed. However, I definitely see this as a limitation of the Chrome abstraction layer - and that's where we should resolve it (e.g. if ports don't work then we can still do broadcasting or direct queuing of messages). Either way, yesterday we saw that a devpanel is treated either as a tab or a frame inside a tab - so we don't have any problems using regular messaging there, and we can solve the hard issues later. Additional API is definitely unnecessary at this point. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... ext/devtools.js:99: }); On 2015/01/10 08:53:45, Sebastian Noack wrote: > This would makes things significantly more complex. And I don't see any > advantage. This is just that you can run the UI standalone. Yes, this is just us testing that thing properly and independently of any browser ;) Given that we don't need any special messaging after all, I guess that there is no issue with implementing the messages in the background page mock?
http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... File devtools-panel.html (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:1: <html> On 2015/01/12 11:55:07, Sebastian Noack wrote: > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > License header > You are right. I wasn't aware that we add the GPL header also to HTML files. > > > and document type are missing > It's optional in HTML5. And I personally prefer to omit. But it seems that we > usually still specify the doctype declaration. So in favor of consistence, I > added it. Thank you. Just one more thing: We changed the protocol in the URL to "https" in the other license headers. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:9: <div id="header"> On 2015/01/12 11:55:07, Sebastian Noack wrote: > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > The structure is a bit scattered so instead of > > > > header > > - table (contains selects as well as table headers) > > - tr > > - td > > div#items > > - table > > - tr > > - td > > footer > > > > I'd suggest to merge the two tables into a single one > > > > header (contains selects) > > table > > - thead (contains table headers) > > - tr > > - th > > - tbody#items > > - tr > > - td > > footer > > Initially I used that structure. But flexbox requires an additional container > around the the table header and content area. That's a pity and seems like that's also how the DevTools implemented it so should be fine for us as well. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... File devtools-panel.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:62: urlElement.innerHTML = " "; On 2015/01/12 11:55:07, Sebastian Noack wrote: > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > I assume you do this to ensure a minimum height for this element. If so you > > should rather use the `min-height` CSS property. > Doesn't seem to do the trick when using flexbox. That's strange because it works for me when emptying the content of this DIV and adding the style `.url {min-height: 17px;}` to set the minimum height to the same value as the defined line height. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:103: urlElement.addEventListener("click", function() On 2015/01/12 11:55:07, Sebastian Noack wrote: > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > No need to add an event listener to each row individually. You can catch the > > event higher up in the hierarchy. > > I know. But then I'd need to double check whether the element clicked on is an > action button, and extract the URL from the DOM. It's much simpler this way. And > I don't care that too many event listeners slow down the page in IE6. I'd agree if `urlElement` had child elements that we'd have to consider. But it doesn't appear much more complicated to, instead of: if ("openResource" in ext.devtools.panel && request.type != "POPUP") { urlElement.classList.add("resourceLink"); urlElement.addEventListener("click", function() { ext.devtools.panels.openResource(request.url); }, false); } write: if (request.type != "POPUP") urlElement.dataset.resourceUrl = request.url; and somewhere else: if ("openResource" in ext.devtools.panels) { document.addEventListener("click", function(ev) { var resourceUrl = ev.target.dataset.resourceUrl; if (resourceUrl) ext.devtools.panels.openResource(resourceUrl); }, true); } You could even use that attribute in your CSS instead of `.resourceLink` by writing `[data-resource-url]`. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... File skin/devtools-panel.css (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... skin/devtools-panel.css:12: display: flex; On 2015/01/12 11:55:07, Sebastian Noack wrote: > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > According to http://caniuse.com/#search=flex Chrome 28 still requires the > > "-webkit" prefix for flexbox properties/values. > > Yep, we are dropping support for Chrome 28 (see the other review). According to > Wladimir only 0.06% are using Chrome 28. Ok, thanks for the info. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... skin/devtools-panel.css:133: text-decoration: underline; On 2015/01/12 11:55:07, Sebastian Noack wrote: > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > It'd be great to make the entire cell clickable and not just the link (as it > is > > in the Networks panel). > > I disagree. Making the document domain clickable would be confusing, since some > users might expect to get taken to the parent document. It already is confusing since the upper half of the cell is clickable and the lower half isn't (try hover over the area in a row above the request type). While my personal preference would be to therefore make the whole cell clickable but I'd also agree to simply limiting the clickable area to the link text itself (excluding the rest of element `div.resourceLink`).
http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... File devtools-panel.html (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.html:1: <html> On 2015/01/16 18:17:21, Thomas Greiner wrote: > On 2015/01/12 11:55:07, Sebastian Noack wrote: > > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > > License header > > You are right. I wasn't aware that we add the GPL header also to HTML files. > > > > > and document type are missing > > It's optional in HTML5. And I personally prefer to omit. But it seems that we > > usually still specify the doctype declaration. So in favor of consistence, I > > added it. > > Thank you. Just one more thing: We changed the protocol in the URL to "https" in > the other license headers. Done. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... File devtools-panel.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:62: urlElement.innerHTML = " "; On 2015/01/16 18:17:21, Thomas Greiner wrote: > On 2015/01/12 11:55:07, Sebastian Noack wrote: > > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > > I assume you do this to ensure a minimum height for this element. If so you > > > should rather use the `min-height` CSS property. > > Doesn't seem to do the trick when using flexbox. > > That's strange because it works for me when emptying the content of this DIV and > adding the style `.url {min-height: 17px;}` to set the minimum height to the > same value as the defined line height. I don't recall anymore what the particular problem was here. But hard-coding the min-height for the cell, would be just another hack, and IMO not any better than using " ". http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:103: urlElement.addEventListener("click", function() On 2015/01/16 18:17:21, Thomas Greiner wrote: > On 2015/01/12 11:55:07, Sebastian Noack wrote: > > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > > No need to add an event listener to each row individually. You can catch the > > > event higher up in the hierarchy. > > > > I know. But then I'd need to double check whether the element clicked on is an > > action button, and extract the URL from the DOM. It's much simpler this way. > And > > I don't care that too many event listeners slow down the page in IE6. > > I'd agree if `urlElement` had child elements that we'd have to consider. But it > doesn't appear much more complicated to, instead of: > > if ("openResource" in ext.devtools.panel && request.type != "POPUP") > { > urlElement.classList.add("resourceLink"); > urlElement.addEventListener("click", function() > { > ext.devtools.panels.openResource(request.url); > }, false); > } > > write: > > if (request.type != "POPUP") > urlElement.dataset.resourceUrl = request.url; > > and somewhere else: > > if ("openResource" in ext.devtools.panels) > { > document.addEventListener("click", function(ev) > { > var resourceUrl = ev.target.dataset.resourceUrl; > if (resourceUrl) > ext.devtools.panels.openResource(resourceUrl); > }, true); > } > > You could even use that attribute in your CSS instead of `.resourceLink` by > writing `[data-resource-url]`. I'd still need to check whether ext.devtools.panels.openResource is available when creating the element. Otherwise it would still look like a link even when you can't actually click it on old Chrome versions. This really is just premature optimization, adding complexity and duplicating logic. Not that, it would make the code any easier to read, when having the event listener defined in an entire different place than the rest of the DOM code related to the element. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... File ext/devtools.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... ext/devtools.js:25: addListener: function(listener) On 2015/01/13 07:28:04, Wladimir Palant wrote: > On 2015/01/10 08:53:45, Sebastian Noack wrote: > > Yes, it will be fairly complicated. Ports are very low-level, and doesn't > > support sending responses to individual messages. > > I see, that's making things too complicated indeed. However, I definitely see > this as a limitation of the Chrome abstraction layer - and that's where we > should resolve it (e.g. if ports don't work then we can still do broadcasting or > direct queuing of messages). Either way, yesterday we saw that a devpanel is > treated either as a tab or a frame inside a tab - so we don't have any problems > using regular messaging there, and we can solve the hard issues later. > Additional API is definitely unnecessary at this point. As discussed some weeks ago, we can actually use regular messaging here, since it turned out that the devtool panels actually has a tab/page. So I'm reusing the mechanism for the first run page now, to have the messaging properly emulated. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/ext/... ext/devtools.js:99: }); On 2015/01/13 07:28:04, Wladimir Palant wrote: > On 2015/01/10 08:53:45, Sebastian Noack wrote: > > This would makes things significantly more complex. And I don't see any > > advantage. This is just that you can run the UI standalone. > > Yes, this is just us testing that thing properly and independently of any > browser ;) > > Given that we don't need any special messaging after all, I guess that there is > no issue with implementing the messages in the background page mock? Yep, done, see above. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... File skin/devtools-panel.css (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/skin... skin/devtools-panel.css:133: text-decoration: underline; On 2015/01/16 18:17:21, Thomas Greiner wrote: > On 2015/01/12 11:55:07, Sebastian Noack wrote: > > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > > It'd be great to make the entire cell clickable and not just the link (as it > > is > > > in the Networks panel). > > > > I disagree. Making the document domain clickable would be confusing, since > some > > users might expect to get taken to the parent document. > > It already is confusing since the upper half of the cell is clickable and the > lower half isn't (try hover over the area in a row above the request type). > > While my personal preference would be to therefore make the whole cell clickable > but I'd also agree to simply limiting the clickable area to the link text itself > (excluding the rest of element `div.resourceLink`). This changed when I addressed Sven's feedback. The type has now a separate column.
http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... File devtools-panel.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:62: urlElement.innerHTML = " "; On 2015/02/04 14:17:29, Sebastian Noack wrote: > On 2015/01/16 18:17:21, Thomas Greiner wrote: > > On 2015/01/12 11:55:07, Sebastian Noack wrote: > > > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > > > I assume you do this to ensure a minimum height for this element. If so > you > > > > should rather use the `min-height` CSS property. > > > Doesn't seem to do the trick when using flexbox. > > > > That's strange because it works for me when emptying the content of this DIV > and > > adding the style `.url {min-height: 17px;}` to set the minimum height to the > > same value as the defined line height. > > I don't recall anymore what the particular problem was here. But hard-coding the > min-height for the cell, would be just another hack, and IMO not any better than > using " ". Assuring that the element has a minimum height is what you're trying to achieve here. Modifying the DOM to work around a styling issue is a hack. http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:103: urlElement.addEventListener("click", function() On 2015/02/04 14:17:29, Sebastian Noack wrote: > On 2015/01/16 18:17:21, Thomas Greiner wrote: > > On 2015/01/12 11:55:07, Sebastian Noack wrote: > > > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > > > No need to add an event listener to each row individually. You can catch > the > > > > event higher up in the hierarchy. > > > > > > I know. But then I'd need to double check whether the element clicked on is > an > > > action button, and extract the URL from the DOM. It's much simpler this way. > > And > > > I don't care that too many event listeners slow down the page in IE6. > > > > I'd agree if `urlElement` had child elements that we'd have to consider. But > it > > doesn't appear much more complicated to, instead of: > > > > if ("openResource" in ext.devtools.panel && request.type != "POPUP") > > { > > urlElement.classList.add("resourceLink"); > > urlElement.addEventListener("click", function() > > { > > ext.devtools.panels.openResource(request.url); > > }, false); > > } > > > > write: > > > > if (request.type != "POPUP") > > urlElement.dataset.resourceUrl = request.url; > > > > and somewhere else: > > > > if ("openResource" in ext.devtools.panels) > > { > > document.addEventListener("click", function(ev) > > { > > var resourceUrl = ev.target.dataset.resourceUrl; > > if (resourceUrl) > > ext.devtools.panels.openResource(resourceUrl); > > }, true); > > } > > > > You could even use that attribute in your CSS instead of `.resourceLink` by > > writing `[data-resource-url]`. > > I'd still need to check whether ext.devtools.panels.openResource is available > when creating the element. Otherwise it would still look like a link even when > you can't actually click it on old Chrome versions. That makes sense. Separating the logic from the view creation would've been nice but since it's not always clickable too much of an overhead, true. http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/back... File background.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/back... background.js:213: // whiletisted request Replace "whiletisted" with "whitelisted" http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/devt... File devtools-panel.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/devt... devtools-panel.js:46: }); We always explicitly set the third parameter when calling addEventListener. http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/devt... devtools-panel.js:59: var urlElement = row.querySelector(".url"); There's no added value in having those extra spaces. http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/devt... devtools-panel.js:112: var template = document.querySelector("template").content.firstElementChild; According to https://developer.mozilla.org/en-US/docs/Web/API/ParentNode/firstElementChild... "firstElement" is not supported on "DocumentFragment" in Chrome 28 which we currently still support. http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/devt... devtools-panel.js:157: }); We always explicitly set the third parameter when calling addEventListener. http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/ext/... File ext/content.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/ext/... ext/content.js:47: That extra line is not really adding any value.
http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... File devtools-panel.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5629499534213120/devt... devtools-panel.js:62: urlElement.innerHTML = " "; On 2015/03/13 11:11:05, Thomas Greiner wrote: > On 2015/02/04 14:17:29, Sebastian Noack wrote: > > On 2015/01/16 18:17:21, Thomas Greiner wrote: > > > On 2015/01/12 11:55:07, Sebastian Noack wrote: > > > > On 2015/01/12 10:37:54, Thomas Greiner wrote: > > > > > I assume you do this to ensure a minimum height for this element. If so > > you > > > > > should rather use the `min-height` CSS property. > > > > Doesn't seem to do the trick when using flexbox. > > > > > > That's strange because it works for me when emptying the content of this DIV > > and > > > adding the style `.url {min-height: 17px;}` to set the minimum height to the > > > same value as the defined line height. > > > > I don't recall anymore what the particular problem was here. But hard-coding > the > > min-height for the cell, would be just another hack, and IMO not any better > than > > using " ". > > Assuring that the element has a minimum height is what you're trying to achieve > here. Modifying the DOM to work around a styling issue is a hack. I don't want a minimum height, I want each line to be exact 17px large in height. Therefore I use the line-height property of the table. However, line-height is only applied to text. So I have to make every line contain some text, that's what I have to use the non-breaking-space for. IMO this is certainly better and not more a hack than redundantly specifying the size with the min-height property for all potential empty <div> elements. http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/back... File background.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/back... background.js:213: // whiletisted request On 2015/03/13 11:11:05, Thomas Greiner wrote: > Replace "whiletisted" with "whitelisted" Done. http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/devt... File devtools-panel.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/devt... devtools-panel.js:46: }); On 2015/03/13 11:11:05, Thomas Greiner wrote: > We always explicitly set the third parameter when calling addEventListener. Well, not always. And frankly, I personally find code adding a parameter to a function call after an anonymous function definition kinda ugly. And don't tell me that not all browsers support omitting the third argument, because it's optional ever since in Chrome and Safari. But yeah, at least in adblockplusui we seem to consistently specify the third argument always, so fair enough. http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/devt... devtools-panel.js:59: var urlElement = row.querySelector(".url"); On 2015/03/13 11:11:05, Thomas Greiner wrote: > There's no added value in having those extra spaces. IMO it is. Code reads quicker and typos become more obvious when identical parts of similar lines are aligned. But yeah, we usually don't do that. So removing the spaces for consistency. http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/devt... devtools-panel.js:112: var template = document.querySelector("template").content.firstElementChild; On 2015/03/13 11:11:05, Thomas Greiner wrote: > According to > https://developer.mozilla.org/en-US/docs/Web/API/ParentNode/firstElementChild... > "firstElement" is not supported on "DocumentFragment" in Chrome 28 which we > currently still support. Again, with this change we drop support for Chrome 28. http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/devt... devtools-panel.js:157: }); On 2015/03/13 11:11:05, Thomas Greiner wrote: > We always explicitly set the third parameter when calling addEventListener. Done. http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/ext/... File ext/content.js (right): http://codereview.adblockplus.org/5646124035604480/diff/5686306919153664/ext/... ext/content.js:47: On 2015/03/13 11:11:05, Thomas Greiner wrote: > That extra line is not really adding any value. Done.
LGTM
I had to rebase, and while on it I enabled strict mode for new files.
LGTM again 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
