Issue 10544195: Show an extension info bubble when a script badge is clicked.

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+315 lines, -38 lines)			Patch
M	chrome/app/generated_resources.grd	View		1 chunk	+8 lines, -0 lines	0 comments	Download
M	chrome/browser/browser_resources.grd	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/extensions/extension_process_manager.cc	View		1 chunk	+5 lines, -1 line	0 comments	Download
M	chrome/browser/extensions/location_bar_controller.h	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/extensions/script_badge_controller.cc	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+1 line, -0 lines	0 comments	Download
A	chrome/browser/resources/extensions/extension_info.css	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+44 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/extensions/extension_info.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+31 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/extensions/extension_info.js	View	1 2 3 4 5 6 7	1 chunk	+23 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/cocoa/location_bar/page_action_decoration.h	View	1 2	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm	View	1 2	4 chunks	+21 lines, -13 lines	0 comments	Download
M	chrome/browser/ui/gtk/location_bar_view_gtk.cc	View	1 2 3 4 5 6 7 8 9 10 11	2 chunks	+9 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/location_bar/page_action_image_view.h	View	1 2	1 chunk	+3 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/views/location_bar/page_action_image_view.cc	View	1 2	4 chunks	+24 lines, -20 lines	0 comments	Download
M	chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc	View	1 2 3 4 5 6 7 8 9 10 11	4 chunks	+13 lines, -1 line	0 comments	Download
A	chrome/browser/ui/webui/extensions/extension_info_ui.h	View	1 2 3 4 5 6 7 8 9	1 chunk	+37 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/webui/extensions/extension_info_ui.cc	View	1 2 3 4 5 6 7 8 9 10	1 chunk	+75 lines, -0 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/common/extensions/extension_constants.cc	View		4 chunks	+8 lines, -3 lines	0 comments	Download
M	chrome/common/url_constants.h	View	1 2 3 4 5 6 7 8 9 10 11	2 chunks	+2 lines, -0 lines	0 comments	Download
M	chrome/common/url_constants.cc	View	1 2 3 4 5 6 7 8 9 10 11	2 chunks	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 25 (0 generated)

Expand Messages | Collapse Messages

Yoyo Zhou

1. I'm not sure if there's a bug for this feature yet; feel free to ...

8 years, 6 months ago (2012-06-15 23:12:43 UTC) #1

Aaron Boodman

Can you also add some more pixels in the gutter between the icon and the ...

8 years, 6 months ago (2012-06-16 02:21:18 UTC) #2

Aaron Boodman

http://codereview.chromium.org/10544195/diff/1/chrome/browser/browser_resources.grd File chrome/browser/browser_resources.grd (right): http://codereview.chromium.org/10544195/diff/1/chrome/browser/browser_resources.grd#newcode63 chrome/browser/browser_resources.grd:63: <include name="IDR_EXTENSION_INFO_JS" file="resources\extensions\extension_info.js" flattenhtml="true" type="BINDATA" /> On 2012/06/16 02:21:19, ...

8 years, 6 months ago (2012-06-16 02:22:36 UTC) #3

Yoyo Zhou

http://codereview.chromium.org/10544195/diff/1/chrome/browser/resources/extensions/extension_info.css File chrome/browser/resources/extensions/extension_info.css (right): http://codereview.chromium.org/10544195/diff/1/chrome/browser/resources/extensions/extension_info.css#newcode1 chrome/browser/resources/extensions/extension_info.css:1: body { On 2012/06/16 02:21:19, Aaron Boodman wrote: > ...

8 years, 6 months ago (2012-06-18 19:39:37 UTC) #4

Yoyo Zhou

Oh, I remembered that this is currently a gtk-only change and will break on the ...

8 years, 6 months ago (2012-06-18 19:40:13 UTC) #5

Yoyo Zhou

On 2012/06/18 23:33:43, Aaron Boodman wrote: > Don't forget about the pixels! I added 2 ...

8 years, 6 months ago (2012-06-19 21:28:31 UTC) #7

Aaron Boodman

lgtm w/ nits http://codereview.chromium.org/10544195/diff/13002/chrome/browser/browser_resources.grd File chrome/browser/browser_resources.grd (right): http://codereview.chromium.org/10544195/diff/13002/chrome/browser/browser_resources.grd#newcode61 chrome/browser/browser_resources.grd:61: <include name="IDR_EXTENSION_INFO_CSS" file="resources\extensions\extension_info.css" flattenhtml="true" type="BINDATA" /> ...

8 years, 6 months ago (2012-06-19 21:42:43 UTC) #8

Yoyo Zhou

Hmm, it's not popping up on Mac. Will need to investigate. http://codereview.chromium.org/10544195/diff/13002/chrome/browser/resources/extensions/extension_info.css File chrome/browser/resources/extensions/extension_info.css (right): ...

8 years, 6 months ago (2012-06-19 22:03:33 UTC) #9

Yoyo Zhou

On 2012/06/19 22:03:33, Yoyo Zhou wrote: > Hmm, it's not popping up on Mac. Will ...

8 years, 6 months ago (2012-06-19 22:05:09 UTC) #10

Yoyo Zhou

estade: please review webui and gtk thakis: please review cocoa sky: please review views

8 years, 6 months ago (2012-06-19 22:54:56 UTC) #12

Evan Stade

https://chromiumcodereview.appspot.com/10544195/diff/25001/chrome/browser/resources/extensions/extension_info.js File chrome/browser/resources/extensions/extension_info.js (right): https://chromiumcodereview.appspot.com/10544195/diff/25001/chrome/browser/resources/extensions/extension_info.js#newcode15 chrome/browser/resources/extensions/extension_info.js:15: chrome.send('requestExtensionInfo', [extensionId]); this will cause flicker. Instead of loading ...

8 years, 6 months ago (2012-06-19 23:50:02 UTC) #14

Yoyo Zhou

8 years, 6 months ago (2012-06-20 00:24:29 UTC) #15

Evan Stade

8 years, 6 months ago (2012-06-20 02:00:07 UTC) #17

Yoyo Zhou

On 2012/06/20 02:00:07, Evan Stade wrote: > https://chromiumcodereview.appspot.com/10544195/diff/25001/chrome/browser/resources/extensions/extension_info.js > File chrome/browser/resources/extensions/extension_info.js (right): > > https://chromiumcodereview.appspot.com/10544195/diff/25001/chrome/browser/resources/extensions/extension_info.js#newcode15 ...

8 years, 6 months ago (2012-06-21 00:21:43 UTC) #18

Evan Stade

cool, lgtm http://codereview.chromium.org/10544195/diff/35002/chrome/browser/resources/extensions/extension_info.css File chrome/browser/resources/extensions/extension_info.css (right): http://codereview.chromium.org/10544195/diff/35002/chrome/browser/resources/extensions/extension_info.css#newcode28 chrome/browser/resources/extensions/extension_info.css:28: font-size: 13px; define fonts in em. http://codereview.chromium.org/10544195/diff/35002/chrome/browser/resources/extensions/extension_info.html ...

8 years, 6 months ago (2012-06-21 18:56:22 UTC) #19

Yoyo Zhou

http://codereview.chromium.org/10544195/diff/35002/chrome/browser/resources/extensions/extension_info.css File chrome/browser/resources/extensions/extension_info.css (right): http://codereview.chromium.org/10544195/diff/35002/chrome/browser/resources/extensions/extension_info.css#newcode28 chrome/browser/resources/extensions/extension_info.css:28: font-size: 13px; On 2012/06/21 18:56:22, Evan Stade wrote: > ...

8 years, 6 months ago (2012-06-21 21:38:05 UTC) #20

Evan Stade

http://codereview.chromium.org/10544195/diff/35002/chrome/browser/resources/extensions/extension_info.html File chrome/browser/resources/extensions/extension_info.html (right): http://codereview.chromium.org/10544195/diff/35002/chrome/browser/resources/extensions/extension_info.html#newcode5 chrome/browser/resources/extensions/extension_info.html:5: <link rel="stylesheet" href="extension_info.css"> On 2012/06/21 21:38:05, Yoyo Zhou wrote: ...

8 years, 6 months ago (2012-06-21 21:46:06 UTC) #21

http://codereview.chromium.org/10544195/diff/35002/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_info.html (right):

http://codereview.chromium.org/10544195/diff/35002/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_info.html:5: <link
rel="stylesheet" href="extension_info.css">
On 2012/06/21 21:38:05, Yoyo Zhou wrote:
> On 2012/06/21 18:56:22, Evan Stade wrote:
> > chrome_shared.css
> 
> Added. It seems silly to do this and then get ugly values for the body margin
> and p line-height that I then have to override. I can understand these
> guidelines for WebUI that fills a whole tab, but for a popup, I'm aiming more
> for the page info popup style.

yea, but chrome_shared has more in it than just those things you stated. You can
ping Glen about this, but I think we do generally want the extra spacing for
multi-line text. That's why it's in chrome_shared.css. I don't know why this
would be different for this popup vs., e.g., chrome://extensions.

http://codereview.chromium.org/10544195/diff/35002/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_info.html:14: <body>
On 2012/06/21 21:38:05, Yoyo Zhou wrote:
> On 2012/06/21 18:56:22, Evan Stade wrote:
> > <body i18n-values=".style.fontFamily:fontfamily;.style.fontSize:fontsize">
> 
> Huh, aa asked me to remove them since they were apparently not doing anything,
> but I'll defer to you on this one.

they do something, but you have to call
ChromeURLDataManager::DataSource::SetFontAndTextDirection() on your dictionary.

http://codereview.chromium.org/10544195/diff/35002/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_info_ui.cc (right):

http://codereview.chromium.org/10544195/diff/35002/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_info_ui.cc:61:
source_->AddString("name", UTF8ToUTF16(extension->name()));
On 2012/06/21 21:38:05, Yoyo Zhou wrote:
> On 2012/06/21 18:56:22, Evan Stade wrote:
> > perhaps use extension->GetBasicInfo
> 
> I was doing that before, but now I'm not using Value anymore, plus we still
have
> to convert to string16.

extension->GetBasicInfo(true, source_->localized_strings());

Yoyo Zhou

8 years, 6 months ago (2012-06-22 19:20:53 UTC) #22

http://codereview.chromium.org/10544195/diff/35002/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_info.html (right):

http://codereview.chromium.org/10544195/diff/35002/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_info.html:5: <link
rel="stylesheet" href="extension_info.css">
On 2012/06/21 21:46:06, Evan Stade wrote:
> On 2012/06/21 21:38:05, Yoyo Zhou wrote:
> > On 2012/06/21 18:56:22, Evan Stade wrote:
> > > chrome_shared.css
> > 
> > Added. It seems silly to do this and then get ugly values for the body
margin
> > and p line-height that I then have to override. I can understand these
> > guidelines for WebUI that fills a whole tab, but for a popup, I'm aiming
more
> > for the page info popup style.
> 
> yea, but chrome_shared has more in it than just those things you stated. You
can
> ping Glen about this, but I think we do generally want the extra spacing for
> multi-line text. That's why it's in chrome_shared.css. I don't know why this
> would be different for this popup vs., e.g., chrome://extensions.

I talked to Glen about this, who advised me that Linux is not a good platform on
which to base decisions like this. We agreed that it looks better with the extra
spacing on Mac, so I'll go with that.

http://codereview.chromium.org/10544195/diff/35002/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extension_info_ui.cc (right):

http://codereview.chromium.org/10544195/diff/35002/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extension_info_ui.cc:61:
source_->AddString("name", UTF8ToUTF16(extension->name()));
On 2012/06/21 21:46:06, Evan Stade wrote:
> On 2012/06/21 21:38:05, Yoyo Zhou wrote:
> > On 2012/06/21 18:56:22, Evan Stade wrote:
> > > perhaps use extension->GetBasicInfo
> > 
> > I was doing that before, but now I'm not using Value anymore, plus we still
> have
> > to convert to string16.
> 
> extension->GetBasicInfo(true, source_->localized_strings());

Ok, good call.

http://codereview.chromium.org/10544195/diff/42001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc (right):

http://codereview.chromium.org/10544195/diff/42001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc:356:
extensions::switch_utils::IsActionBoxEnabled()) {
Since we stopped using this for all the new extensions UI, I changed it to the
more apt AreScriptBadgesEnabled.

Evan Stade

just a reminder -- I think you need to use ChromeURLDataManager::DataSource::SetFontAndTextDirection()

8 years, 6 months ago (2012-06-23 00:56:05 UTC) #23

Yoyo Zhou

On Fri, Jun 22, 2012 at 5:56 PM, <estade@chromium.org> wrote: > just a reminder -- ...

8 years, 6 months ago (2012-06-23 01:08:43 UTC) #24

Evan Stade

8 years, 6 months ago (2012-06-23 01:09:58 UTC) #25

great

-- Evan Stade


On Fri, Jun 22, 2012 at 6:08 PM, Yoyo Zhou <yoz@chromium.org> wrote:

> On Fri, Jun 22, 2012 at 5:56 PM,  <estade@chromium.org> wrote:
> > just a reminder -- I think you need to use
> > ChromeURLDataManager::DataSource::SetFontAndTextDirection()
> >
> > http://codereview.chromium.org/10544195/
>
> It appears to get called in:
>
> #0  ChromeURLDataManager::DataSource::SetFontAndTextDirection
> (localized_strings=0x7fffd8be57f8)
>    at ../../chrome/browser/ui/webui/chrome_url_data_manager.cc:170
> #1  0x00007ffff05e33a8 in
> ChromeWebUIDataSource::SendLocalizedStringsAsJSON
> (this=0x7fffd8be5790,
>    request_id=38) at
> ../../chrome/browser/ui/webui/chrome_web_ui_data_source.cc:78
> #2  0x00007ffff05e327b in ChromeWebUIDataSource::StartDataRequest
> (this=0x7fffd8be5790, path=
>    "strings.js", is_incognito=false, request_id=38)
>

Expand Messages | Collapse Messages

Issue 10544195: Show an extension info bubble when a script badge is clicked. (Closed)

Description

Patch Set 1 #

Patch Set 2 : AA? WHY NOT AAA+? #

Patch Set 3 : pixels & platforms #

Patch Set 4 : 1px #

Patch Set 5 : license #

Patch Set 6 : rename resources #

Patch Set 7 : js docs #

Patch Set 8 : loadTimeData #

Patch Set 9 : cleaner #

Patch Set 10 : estade #

Patch Set 11 : fin #

Patch Set 12 : rebase eh #

Messages