Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
196413028:
Promote Apps Developer Tools in the chrome:extensions page (Closed)
DescriptionPromote Apps Developer Tools in the chrome:extensions page
If a user does not have the Apps Developer Tools installed, display a promotion
for it under the Developer Tools controls (the promo only shows up in Dev Mode).
The promo will go away (or not appear in the first place) if the user has ADT
installed.
BUG=312396
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264906
Patch Set 1 : #Patch Set 2 : Clean CSS, hide behind command-line flag #Patch Set 3 : #
Total comments: 29
Patch Set 4 : Dan's #
Total comments: 8
Patch Set 5 : #
Total comments: 6
Patch Set 6 : Nits #
Messages
Total messages: 17 (0 generated)
Hey Dan, mind taking a look when you can? And I apologize in advance for my css... Pics: https://drive.google.com/a/google.com/folderview?id=0BxjK6_MzZ8UPejZyU2g1TEJy...
who actually designed this?
On 2014/03/29 00:54:40, Dan Beam wrote: > who actually designed this? The UI designed by jstark@, here: crbug.com/354709.
waiting on this: https://code.google.com/p/chromium/issues/detail?id=354709#c13 and possibly more Review-*-Yes flags to be set on the newly targeted bug ;)
On 2014/04/04 03:09:59, Dan Beam wrote: > waiting on this: > https://code.google.com/p/chromium/issues/detail?id=354709#c13 > > and possibly more Review-*-Yes flags to be set on the newly targeted bug ;) meant Launch-*-Yes of course
On 2014/04/04 03:13:00, Dan Beam wrote: > On 2014/04/04 03:09:59, Dan Beam wrote: > > waiting on this: > > https://code.google.com/p/chromium/issues/detail?id=354709#c13 > > > > and possibly more Review-*-Yes flags to be set on the newly targeted bug ;) > > meant Launch-*-Yes of course Yeah, sorry - I thought there was less dissent here. We're discussing in email, and it looks like we should be getting close to a conclusion. In prep, I cleaned up the CSS (though I'm sure it could probably be better) and put it all behind a command line flag. And centered the close button for you. :) http://imgur.com/BnJsoQn I'll ping again when everything's resolved on our end.
On 2014/04/09 23:41:23, D Cronin wrote: > On 2014/04/04 03:13:00, Dan Beam wrote: > > On 2014/04/04 03:09:59, Dan Beam wrote: > > > waiting on this: > > > https://code.google.com/p/chromium/issues/detail?id=354709#c13 > > > > > > and possibly more Review-*-Yes flags to be set on the newly targeted bug ;) > > > > meant Launch-*-Yes of course > > Yeah, sorry - I thought there was less dissent here. We're discussing in email, > and it looks like we should be getting close to a conclusion. In prep, I > cleaned up the CSS (though I'm sure it could probably be better) and put it all > behind a command line flag. And centered the close button for you. :) > http://imgur.com/BnJsoQn > > I'll ping again when everything's resolved on our end. Okay, I think things are resolved on crbug.com/354709. End result is a change in the name of the product, and switching from "Learn more" to "Visit website". Per requests on the bug, this will now be behind a channel guard (<= DEV only), not a command-line flag. Sorry again for all the hassle on this... Is there anything else you'd like to see happen before we push this through for Dev?
https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.css (right): https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:54: content: url('apps_developer_tools_promo_48.png'); nit: no need for single quotes https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:63: text-decoration: none; i think we underline links that go to new hosts https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:74: background-position: center; background-position's initial value is 0% 0% (top-left for both x and y). when you provide an x value, the initial y value is center (go home CSS, you're drunk) [1]. whether you meant to do this or not, it's pretty darn sneaky. tl;dr - list both x & y [1] http://dev.w3.org/csswg/css-backgrounds/#position https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:75: background-repeat: no-repeat; or better yet: background: url(chrome://theme/IDR_CLOSE_DIALOG) no-repeat center center; https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:82: background-image: url('chrome://theme/IDR_CLOSE_DIALOG_H'); ^ does this preload? i.e. does this load when hovering and potentially flicker? https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.html (right): https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.html:92: <img></img> i'm not sure, but this may load an image with src=location.href which may infinitely keep loading the same page... [1] if this kind of wonky behavior still happens, can you just append this <img> dynamically in JS or set some bogus src or something? if not, ignore my stale web development gotchas (it might've been fixed in new HTML/Chrome). [1] http://www.nczonline.net/blog/2009/11/30/empty-image-src-can-destroy-your-site/ https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:119: $('apps-developer-tools-promo').querySelector('.close-button') nit: . at end https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:311: pageDiv.classList.add('adt-promo'); why are we hiding the promo at all? can't we just leave it unhidden but obscured from view until .adt-promo is added/removed? right now it seems like this would immediately hide the promo and transition its parent's height after that... might look a bit jarring. https://codereview.chromium.org/196413028/diff/70001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/196413028/diff/70001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:477: "extensionSettingsAppsDevToolsPromoText", I don't really think the "extensionSettings" prefix is required here (do we have more than one app developer tools in this file/page?) but i guess everything else is prefixed here :_(. https://codereview.chromium.org/196413028/diff/70001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:835: // already. If they do, we shouldn't promote it. nit: // Promote Apps Developer Tools if they're not already installed. https://codereview.chromium.org/196413028/diff/70001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:839: ->GetExtensionById(kAppsDeveloperToolsExtensionId, nit: -> at end, CLANG FORMATTTTTTTTTTTTT! https://codereview.chromium.org/196413028/diff/70001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:840: ExtensionRegistry::EVERYTHING) == NULL; Get(...) == NULL -> !Get(...)
https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.css (right): https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:97: height: 105px; /* Allow more height for the Apps Developer Tools promo. */ also, it'd be kinda cool if we could just somehow dynamically calculate this (e.g. 100% or calc(<orig height> + <extra height>) but I don't know a good way to do this...
https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.css (right): https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:54: content: url('apps_developer_tools_promo_48.png'); On 2014/04/18 21:27:28, Dan Beam wrote: > nit: no need for single quotes Done. https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:63: text-decoration: none; On 2014/04/18 21:27:28, Dan Beam wrote: > i think we underline links that go to new hosts Ah, so we do. Done. https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:74: background-position: center; On 2014/04/18 21:27:28, Dan Beam wrote: > background-position's initial value is 0% 0% (top-left for both x and y). when > you provide an x value, the initial y value is center (go home CSS, you're > drunk) [1]. whether you meant to do this or not, it's pretty darn sneaky. > > tl;dr - list both x & y > > [1] http://dev.w3.org/csswg/css-backgrounds/#position Crazy... done. https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:75: background-repeat: no-repeat; On 2014/04/18 21:27:28, Dan Beam wrote: > or better yet: > > background: url(chrome://theme/IDR_CLOSE_DIALOG) no-repeat center center; Awesome, thanks. https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:82: background-image: url('chrome://theme/IDR_CLOSE_DIALOG_H'); On 2014/04/18 21:27:28, Dan Beam wrote: > ^ does this preload? i.e. does this load when hovering and potentially flicker? I don't see any hover, and this is how we do it elsewhere - but neither of those are exactly definitive. Is there a better alternative to this? https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:97: height: 105px; /* Allow more height for the Apps Developer Tools promo. */ On 2014/04/18 21:28:22, Dan Beam wrote: > also, it'd be kinda cool if we could just somehow dynamically calculate this > (e.g. 100% or calc(<orig height> + <extra height>) but I don't know a good way > to do this... I agree it'd be cool, but it'd require a bit more work (obviously, we do it this way for extending to include #dev-controls, too). It okay to leave this one off for now? https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.html (right): https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.html:92: <img></img> On 2014/04/18 21:27:28, Dan Beam wrote: > i'm not sure, but this may load an image with src=location.href which may > infinitely keep loading the same page... [1] > > if this kind of wonky behavior still happens, can you just append this <img> > dynamically in JS or set some bogus src or something? if not, ignore my stale > web development gotchas (it might've been fixed in new HTML/Chrome). > > [1] > http://www.nczonline.net/blog/2009/11/30/empty-image-src-can-destroy-your-site/ It doesn't look like this happens. So either fixed, or because it doesn't set the src to empty? (The website identified src="" two ways, but didn't address just var i = Image(), or <img></img>). https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:119: $('apps-developer-tools-promo').querySelector('.close-button') On 2014/04/18 21:27:28, Dan Beam wrote: > nit: . at end Whoops, sorry - git cl format is making bad habits for me ;) https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:311: pageDiv.classList.add('adt-promo'); On 2014/04/18 21:27:28, Dan Beam wrote: > why are we hiding the promo at all? can't we just leave it unhidden but > obscured from view until .adt-promo is added/removed? > > right now it seems like this would immediately hide the promo and transition its > parent's height after that... might look a bit jarring. Oh, I guess we can. Nice. https://codereview.chromium.org/196413028/diff/70001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_settings_handler.cc (right): https://codereview.chromium.org/196413028/diff/70001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:477: "extensionSettingsAppsDevToolsPromoText", On 2014/04/18 21:27:28, Dan Beam wrote: > I don't really think the "extensionSettings" prefix is required here (do we have > more than one app developer tools in this file/page?) but i guess everything > else is prefixed here :_(. Yeah, went with consistency over easy typing. :/ Lemme know if you want me to change it, though. https://codereview.chromium.org/196413028/diff/70001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:835: // already. If they do, we shouldn't promote it. On 2014/04/18 21:27:28, Dan Beam wrote: > nit: // Promote Apps Developer Tools if they're not already installed. Done. https://codereview.chromium.org/196413028/diff/70001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:839: ->GetExtensionById(kAppsDeveloperToolsExtensionId, On 2014/04/18 21:27:28, Dan Beam wrote: > nit: -> at end, CLANG FORMATTTTTTTTTTTTT! Haha done. :) https://codereview.chromium.org/196413028/diff/70001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_settings_handler.cc:840: ExtensionRegistry::EVERYTHING) == NULL; On 2014/04/18 21:27:28, Dan Beam wrote: > Get(...) == NULL -> !Get(...) Done (sometimes people have preferences when testing a ptr for nullness, rather than a bool -- I'll try to keep it simple here from now on.) :)
so close https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.css (right): https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:82: background-image: url('chrome://theme/IDR_CLOSE_DIALOG_H'); On 2014/04/18 22:19:55, D Cronin wrote: > On 2014/04/18 21:27:28, Dan Beam wrote: > > ^ does this preload? i.e. does this load when hovering and potentially > flicker? > > I don't see any hover, and this is how we do it elsewhere - but neither of those > are exactly definitive. Is there a better alternative to this? eh, this is local and so small... if you don't notice (or this image is already loaded elsewhere on the page), I wouldn't worry about it. https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:97: height: 105px; /* Allow more height for the Apps Developer Tools promo. */ On 2014/04/18 22:19:55, D Cronin wrote: > On 2014/04/18 21:28:22, Dan Beam wrote: > > also, it'd be kinda cool if we could just somehow dynamically calculate this > > (e.g. 100% or calc(<orig height> + <extra height>) but I don't know a good way > > to do this... > > I agree it'd be cool, but it'd require a bit more work (obviously, we do it this > way for extending to include #dev-controls, too). It okay to leave this one off > for now? yes https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.html (right): https://codereview.chromium.org/196413028/diff/70001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.html:92: <img></img> On 2014/04/18 22:19:55, D Cronin wrote: > On 2014/04/18 21:27:28, Dan Beam wrote: > > i'm not sure, but this may load an image with src=location.href which may > > infinitely keep loading the same page... [1] > > > > if this kind of wonky behavior still happens, can you just append this <img> > > dynamically in JS or set some bogus src or something? if not, ignore my stale > > web development gotchas (it might've been fixed in new HTML/Chrome). > > > > [1] > > > http://www.nczonline.net/blog/2009/11/30/empty-image-src-can-destroy-your-site/ > > It doesn't look like this happens. So either fixed, or because it doesn't set > the src to empty? (The website identified src="" two ways, but didn't address > just var i = Image(), or <img></img>). great, pretty sure this is fixed, then. it wouldn't happen in the new Image() case, either way. https://codereview.chromium.org/196413028/diff/90001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.css (right): https://codereview.chromium.org/196413028/diff/90001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:76: background-image: url('chrome://theme/IDR_CLOSE_DIALOG_H'); Y U STILL 'quoted'?! :_( https://codereview.chromium.org/196413028/diff/90001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/196413028/diff/90001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:122: $('apps-developer-tools-promo').hidden = true; do we actually still need this ^ code? https://codereview.chromium.org/196413028/diff/90001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:123: $('extension-settings').classList.remove('adt-promo'); using the helpful magic mentioned later: this.promoDismissed_ = true; this.updatePromoVisibility_(); https://codereview.chromium.org/196413028/diff/90001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:308: throw this somewhere /** * Updates (hides/shows) the promo based on whether it should be showing. * @private */ updatePromoVisibility_: function() { var showPromo = extensionsData.promoteAppsDevTools && !this.promoDismissed_; $('extension-settings').classList.toggle('adt-promo', showPromo); }, and call from here
https://codereview.chromium.org/196413028/diff/90001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.css (right): https://codereview.chromium.org/196413028/diff/90001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.css:76: background-image: url('chrome://theme/IDR_CLOSE_DIALOG_H'); On 2014/04/18 22:34:23, Dan Beam wrote: > Y U STILL 'quoted'?! :_( Gah!! %s/url('\(.*\)')/url(\1)/gc Sorry about that. https://codereview.chromium.org/196413028/diff/90001/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/196413028/diff/90001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:122: $('apps-developer-tools-promo').hidden = true; On 2014/04/18 22:34:23, Dan Beam wrote: > do we actually still need this ^ code? Whoops, gone. https://codereview.chromium.org/196413028/diff/90001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:123: $('extension-settings').classList.remove('adt-promo'); On 2014/04/18 22:34:23, Dan Beam wrote: > using the helpful magic mentioned later: > > this.promoDismissed_ = true; > this.updatePromoVisibility_(); See below. https://codereview.chromium.org/196413028/diff/90001/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:308: On 2014/04/18 22:34:23, Dan Beam wrote: > throw this somewhere > > /** > * Updates (hides/shows) the promo based on whether it should be showing. > * @private > */ > updatePromoVisibility_: function() { > var showPromo = extensionsData.promoteAppsDevTools && !this.promoDismissed_; > $('extension-settings').classList.toggle('adt-promo', showPromo); > }, > > and call from here extensionsData is a local variable. :( We could make it this.extensionsData_, but that seems a little much for the little it does... preference? In the meantime, updated the if / else to be the var showPromo + toggle, which makes it a bit cleaner.
lgtm https://codereview.chromium.org/196413028/diff/110001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/196413028/diff/110001/chrome/app/generated_re... chrome/app/generated_resources.grd:4683: + Visit website I still think this is a bit funky ("Install" or something might be better, IMO), but I'll let the designers figure this out https://codereview.chromium.org/196413028/diff/110001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/196413028/diff/110001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:87: nit: /** * Whether the App Developer Tools promo has been dismissed on this page. * @type {boolean} * @private */ promoDismissed_: false, https://codereview.chromium.org/196413028/diff/110001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:118: this.promoDismissed_ = false; nit: move to prototype (see above)
https://codereview.chromium.org/196413028/diff/110001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/196413028/diff/110001/chrome/app/generated_re... chrome/app/generated_resources.grd:4683: + Visit website On 2014/04/18 23:08:33, Dan Beam wrote: > I still think this is a bit funky ("Install" or something might be better, IMO), > but I'll let the designers figure this out I think we opted against "Install" because it seemed so definitive - like it would start the action for you, rather than taking you to the webstore page. I agree "Visit website" isn't quite right either, but has the symmetry with the "Visit website" links on each extension. https://codereview.chromium.org/196413028/diff/110001/chrome/browser/resource... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/196413028/diff/110001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:87: On 2014/04/18 23:08:33, Dan Beam wrote: > nit: > > /** > * Whether the App Developer Tools promo has been dismissed on this page. > * @type {boolean} > * @private > */ > promoDismissed_: false, Done. https://codereview.chromium.org/196413028/diff/110001/chrome/browser/resource... chrome/browser/resources/extensions/extensions.js:118: this.promoDismissed_ = false; On 2014/04/18 23:08:33, Dan Beam wrote: > nit: move to prototype (see above) Done.
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/196413028/...
Message was sent while issue was closed.
Change committed as 264906 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
