Add a first-class off-store install UI to chrome://extensions/.

Add a first-class off-store install UI to chrome://extensions/.

This patch implements GTK only. Other ports will be separate CLs.

BUG=55584
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135525

[Aaron Boodman, 2012-05-01 06:52:14]

Note: This depends on switch_utils.* from
https://chromiumcodereview.appspot.com/10266015/

[Aaron Boodman, 2012-05-01 06:54:28]

And here is what it looks like:

http://chromium.googlecode.com/issues/attachment?aid=1207560010000&name=drag-...

http://chromium.googlecode.com/issues/attachment?aid=1207560010001&name=drag-...

[Aaron Boodman, 2012-05-01 06:56:16]

Nuts: http://imgur.com/VsSAg,h795B

[Aaron Boodman, 2012-05-01 15:16:42]

+jam for content

[jam, 2012-05-01 19:08:58]

content lgtm
+estade for his thoughts on adding these methods to WebUIController, or if
there's any other way of doing this

[Evan Stade, 2012-05-01 19:33:54]

I don't understand the need for those new WebUIController functions at all. They
are not triggered for a particularly special type of drag, they are triggered
for all drags. Why not just use the normal html5 drag events?

[Aaron Boodman, 2012-05-01 20:09:54]

On 2012/05/01 19:33:54, Evan Stade wrote:
> I don't understand the need for those new WebUIController functions at all.
They
> are not triggered for a particularly special type of drag, they are triggered
> for all drags. Why not just use the normal html5 drag events?

Yeah, good question. Two reasons:

1) I could not get the 'dragleave' event to fire reliably in the uber page, so
that I could know when to remove the drop target. Clearly this must work somehow
because, e.g., Gmail pulls it off. I think it might have to do with the framed
construction of the uber page.

2) For security, we don't want to allow the HTML in the uber page to specify the
crx to install. It should just say 'install the file that was previously being
dragged'.

[Evan Stade, 2012-05-01 20:27:18]

On 2012/05/01 20:09:54, Aaron Boodman wrote:
> On 2012/05/01 19:33:54, Evan Stade wrote:
> > I don't understand the need for those new WebUIController functions at all.
> They
> > are not triggered for a particularly special type of drag, they are
triggered
> > for all drags. Why not just use the normal html5 drag events?
> 
> Yeah, good question. Two reasons:
> 
> 1) I could not get the 'dragleave' event to fire reliably in the uber page, so
> that I could know when to remove the drop target. Clearly this must work
somehow
> because, e.g., Gmail pulls it off. I think it might have to do with the framed
> construction of the uber page.

really? I can't get it to act unreliably. This is the test code I just added to
ExtensionSettings::initialize()

      var body = document.body;
      var count = 0;
      body.addEventListener('dragenter', function(e) { count++;
          if (count == 1)
            console.error(' >>>> ENTER');
      });
      body.addEventListener('dragleave', function(e) {
          count--;
          if (count == 0)
            console.error(' <<<< LEAVE');
      });

and it produces perfectly interleaved ENTER/LEAVE messages no matter how
vigorously I swirl my mouse around. (You might be interested in the DragWrapper
class found in shared/ which handles the counting for you.)

> 
> 2) For security, we don't want to allow the HTML in the uber page to specify
the
> crx to install. It should just say 'install the file that was previously being
> dragged'.

yea, I can see the need for keeping the actual drag data on the c++ side, but
that doesn't apply to the new WebUIController functions.

[Aaron Boodman, 2012-05-01 20:55:41]

On 2012/05/01 20:27:18, Evan Stade wrote:
> On 2012/05/01 20:09:54, Aaron Boodman wrote:
> > On 2012/05/01 19:33:54, Evan Stade wrote:
> > > I don't understand the need for those new WebUIController functions at
all.
> > They
> > > are not triggered for a particularly special type of drag, they are
> triggered
> > > for all drags. Why not just use the normal html5 drag events?
> > 
> > Yeah, good question. Two reasons:
> > 
> > 1) I could not get the 'dragleave' event to fire reliably in the uber page,
so
> > that I could know when to remove the drop target. Clearly this must work
> somehow
> > because, e.g., Gmail pulls it off. I think it might have to do with the
framed
> > construction of the uber page.
> 
> really? I can't get it to act unreliably. This is the test code I just added
to
> ExtensionSettings::initialize()
> 
>       var body = document.body;
>       var count = 0;
>       body.addEventListener('dragenter', function(e) { count++;
>           if (count == 1)
>             console.error(' >>>> ENTER');
>       });
>       body.addEventListener('dragleave', function(e) {
>           count--;
>           if (count == 0)
>             console.error(' <<<< LEAVE');
>       });
> 
> and it produces perfectly interleaved ENTER/LEAVE messages no matter how
> vigorously I swirl my mouse around. (You might be interested in the
DragWrapper
> class found in shared/ which handles the counting for you.)

Thanks. I tried this approach, and it didn't seem to work correctly. I must have
had a bug, but since I felt like I needed the C++ changes for problem #2, I
didn't try and get it working that hard.

> > 2) For security, we don't want to allow the HTML in the uber page to specify
> the
> > crx to install. It should just say 'install the file that was previously
being
> > dragged'.
> 
> yea, I can see the need for keeping the actual drag data on the c++ side, but
> that doesn't apply to the new WebUIController functions.

OK, if I can rely on HTML5 drag drop (and it looks like I can), then the C++
changes can be simpler. I could just add WebContentsView::GetWebDropData(),
which would be implemented in each platform version of WebContentsView by poking
at the contained WebDragDest.

Jam, WDYT?

[Aaron Boodman, 2012-05-01 22:52:56]

On 2012/05/01 20:55:41, Aaron Boodman wrote:
> On 2012/05/01 20:27:18, Evan Stade wrote:
> > On 2012/05/01 20:09:54, Aaron Boodman wrote:
> OK, if I can rely on HTML5 drag drop (and it looks like I can), then the C++
> changes can be simpler. I could just add WebContentsView::GetWebDropData(),
> which would be implemented in each platform version of WebContentsView by
poking
> at the contained WebDragDest.
> 
> Jam, WDYT?

PTAL. My latest patch uses DragWrapper and pulls WebDragData out of
WebContentsView when needed, rather than pushing it into WebUIController all the
time.

[James Hawkins, 2012-05-01 23:13:35]

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.js (right):

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:64: shouldAcceptDrag:
function() {
nit: Document these methods.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extensions_ui.cc (right):

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extensions_ui.cc:34:
ExtensionsUI::ExtensionsUI(content::WebUI* web_ui)
This line change appears unnecessary.  Can you revert it?

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/install_extension_handler.cc (right):

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.cc:31:
localized_strings->SetString("extensionSettingsInstallDropTarget",
nit: The start of parameter rows must all begin on the same column.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.cc:56: LOG(ERROR)
<< "No current drop data.";
Remove log spam?

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.cc:65:
file_to_install_ = FilePath::FromWStringHack(
What is one supposed to do here to avoid using the *Hack version?

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/install_extension_handler.h (right):

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.h:13: class
InstallExtensionHandler : public content::WebUIMessageHandler {
nit: Document class.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.h:28: FilePath
file_to_install_;
nit: Document member var.  The var name alone is not enough.

[Evan Stade, 2012-05-01 23:43:06]

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.js (right):

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:64: shouldAcceptDrag:
function() {
On 2012/05/01 23:13:36, James Hawkins wrote:
> nit: Document these methods.

@inheritDoc is enough

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:66: // but the event always
says there are no files at this point.
what types are available in dataTransfer? it should be fully populated by this
point. dataTransfer.files may not be implemented in webkit, but you can probably
get at the path as a file URL (and check if it ends in crx)

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:71: var dropTarget =
$('install-drop-target');
what I had intended was

body.drag-target #install-drop-target {
  opacity: 1;
}

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:89: e.preventDefault();
this breaks the case where you drag a URL to the page (which should navigate)

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/install_extension_handler.cc (right):

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.cc:34:
"offstoreinstallenabled",
camel case

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.cc:53: void
InstallExtensionHandler::HandleStartDragMessage(const ListValue* args) {
is this necessary because by the time you get to handleInstallMessage, the drag
data is already reset?

http://codereview.chromium.org/10270031/diff/13001/content/browser/web_conten...
File content/browser/web_contents/web_drag_dest_gtk.h (right):

http://codereview.chromium.org/10270031/diff/13001/content/browser/web_conten...
content/browser/web_contents/web_drag_dest_gtk.h:32: WebDropData*
current_drop_data() const { return drop_data_.get(); }
return const ref

[Aaron Boodman, 2012-05-02 00:41:35]

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.js (right):

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:64: shouldAcceptDrag:
function() {
On 2012/05/01 23:13:36, James Hawkins wrote:
> nit: Document these methods.

To make this more clear, I've extracted the handler out into its own helper
object. With this, I don't think it is worthwhile to comment on each method.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:66: // but the event always
says there are no files at this point.
On 2012/05/01 23:43:07, Evan Stade wrote:
> what types are available in dataTransfer? it should be fully populated by this
> point. dataTransfer.files may not be implemented in webkit, but you can
probably
> get at the path as a file URL (and check if it ends in crx)

Negative. The DataTransfer object is in 'protected mode' at this point and its
contents cannot be accessed:

http://developers.whatwg.org/dnd.html#concept-dnd-p

Added comment.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:71: var dropTarget =
$('install-drop-target');
On 2012/05/01 23:43:07, Evan Stade wrote:
> what I had intended was
> 
> body.drag-target #install-drop-target {
>   opacity: 1;
> }

I did that for display. I need to delay the assignment of opacity in order to
get the transition effect (unless you know of another way).

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:89: e.preventDefault();
On 2012/05/01 23:43:07, Evan Stade wrote:
> this breaks the case where you drag a URL to the page (which should navigate)

Hm, I am not sure it makes sense for navigation to happen when we are showing
the drag effect for all drags, but I've added it anyway because I like you.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/extensions_ui.cc (right):

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/extensions_ui.cc:34:
ExtensionsUI::ExtensionsUI(content::WebUI* web_ui)
On 2012/05/01 23:13:36, James Hawkins wrote:
> This line change appears unnecessary.  Can you revert it?

Done.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/install_extension_handler.cc (right):

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.cc:31:
localized_strings->SetString("extensionSettingsInstallDropTarget",
On 2012/05/01 23:13:36, James Hawkins wrote:
> nit: The start of parameter rows must all begin on the same column.

Done.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.cc:34:
"offstoreinstallenabled",
On 2012/05/01 23:43:07, Evan Stade wrote:
> camel case

HTML attributes are forced to lowercase, so this does nothing. But I have done
it anyway because I like you.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.cc:53: void
InstallExtensionHandler::HandleStartDragMessage(const ListValue* args) {
On 2012/05/01 23:43:07, Evan Stade wrote:
> is this necessary because by the time you get to handleInstallMessage, the
drag
> data is already reset?

Yep. Added comment.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.cc:56: LOG(ERROR)
<< "No current drop data.";
On 2012/05/01 23:13:36, James Hawkins wrote:
> Remove log spam?

Disagree that it is log spam. This will only occur when something unexpected has
genuinely occurred. It won't happen in normal usage.

I have changed it to DLOG, though.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.cc:65:
file_to_install_ = FilePath::FromWStringHack(
On 2012/05/01 23:13:36, James Hawkins wrote:
> What is one supposed to do here to avoid using the *Hack version?

You're not supposed to have a string16 in the first place, which isn't an option
for me.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
File chrome/browser/ui/webui/extensions/install_extension_handler.h (right):

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.h:13: class
InstallExtensionHandler : public content::WebUIMessageHandler {
On 2012/05/01 23:13:36, James Hawkins wrote:
> nit: Document class.

Done.

http://codereview.chromium.org/10270031/diff/13001/chrome/browser/ui/webui/ex...
chrome/browser/ui/webui/extensions/install_extension_handler.h:28: FilePath
file_to_install_;
On 2012/05/01 23:13:36, James Hawkins wrote:
> nit: Document member var.  The var name alone is not enough.

Done.

http://codereview.chromium.org/10270031/diff/13001/content/browser/web_conten...
File content/browser/web_contents/web_drag_dest_gtk.h (right):

http://codereview.chromium.org/10270031/diff/13001/content/browser/web_conten...
content/browser/web_contents/web_drag_dest_gtk.h:32: WebDropData*
current_drop_data() const { return drop_data_.get(); }
On 2012/05/01 23:43:07, Evan Stade wrote:
> return const ref

It can be NULL (when there is no drag in progress). I could return const *, but
I think just returning the pointer is more typical.

[jam, 2012-05-02 00:47:36]

On 2012/05/01 22:52:56, Aaron Boodman wrote:
> On 2012/05/01 20:55:41, Aaron Boodman wrote:
> > On 2012/05/01 20:27:18, Evan Stade wrote:
> > > On 2012/05/01 20:09:54, Aaron Boodman wrote:
> > OK, if I can rely on HTML5 drag drop (and it looks like I can), then the C++
> > changes can be simpler. I could just add WebContentsView::GetWebDropData(),
> > which would be implemented in each platform version of WebContentsView by
> poking
> > at the contained WebDragDest.
> > 
> > Jam, WDYT?
> 
> PTAL. My latest patch uses DragWrapper and pulls WebDragData out of
> WebContentsView when needed, rather than pushing it into WebUIController all
the
> time.

nice that you can do this in JS.

my question is why involve C++ at all? you mention security, but we control the
uber page so this shouldn't be a problem, right? if we don't trust our webui
pages, it seems we're really hosed?

an alternative would be to get the filenames from JS but verify them in C++,
that way you don't need to add new APIs to content/public. i.e. use
ChildProcessSecurityPolicy perhaps?

[Aaron Boodman, 2012-05-02 00:56:15]

On Tue, May 1, 2012 at 5:47 PM,  <jam@chromium.org> wrote:
> my question is why involve C++ at all? you mention security, but we control
> the
> uber page so this shouldn't be a problem, right? if we don't trust our webui
> pages, it seems we're really hosed?

We don't trust our web ui's. We have had a history of XSS's against
them, and the ability to install extensions is a very powerful
privilege that we should be careful of handing out.  One of the recent
Pwnium exploits involved this exact scenario.

Further, there is no reason to say "If we don't trust our webui pages,
we are really hosted". Why not grant them only the privileges they
need to do their jobs?

> an alternative would be to get the filenames from JS but verify them in C++,
> that way you don't need to add new APIs to content/public. i.e. use
> ChildProcessSecurityPolicy perhaps?

Can you flesh out how that would work? I don't follow.

- a

[jam, 2012-05-02 18:00:05]

On 2012/05/02 00:56:15, Aaron Boodman wrote:
> On Tue, May 1, 2012 at 5:47 PM,  <mailto:jam@chromium.org> wrote:
> > my question is why involve C++ at all? you mention security, but we control
> > the
> > uber page so this shouldn't be a problem, right? if we don't trust our webui
> > pages, it seems we're really hosed?
> 
> We don't trust our web ui's. We have had a history of XSS's against
> them, and the ability to install extensions is a very powerful
> privilege that we should be careful of handing out.  One of the recent
> Pwnium exploits involved this exact scenario.
> 
> Further, there is no reason to say "If we don't trust our webui pages,
> we are really hosted". Why not grant them only the privileges they
> need to do their jobs?
> 
> > an alternative would be to get the filenames from JS but verify them in C++,
> > that way you don't need to add new APIs to content/public. i.e. use
> > ChildProcessSecurityPolicy perhaps?
> 
> Can you flesh out how that would work? I don't follow.
> 
> - a

I was saying get the filename from JS, and verify that the user dragged it by
using ChildProcessSecurityPolicy

RenderViewHostImpl::DragTargetDragEnter calls
ChildProcessSecurityPolicy::GrantReadFile

but then looking into it deeper, I see now we don't expose
ChildProcessSecurityPolicy::CanReadFile. it seems if we expose that in the
interface instead, that's exposing less in the Content API than the WebDropData
object

[Evan Stade, 2012-05-02 20:23:43]

I tried to download this and apply it but compile didn't work. Did you forget to
git add chrome/common/extensions/switch_utils.*?

[Aaron Boodman, 2012-05-02 20:35:10]

On Wed, May 2, 2012 at 1:23 PM,  <estade@chromium.org> wrote:
> I tried to download this and apply it but compile didn't work. Did you
> forget to
> git add chrome/common/extensions/switch_utils.*?
>
> http://codereview.chromium.org/10270031/

Like I said, it relies upon an earlier CL that was also out for
review. I've landed that earlier CL now, so it should apply.

[Evan Stade, 2012-05-02 22:03:09]

> Like I said, it relies upon an earlier CL that was also out for
> review. I've landed that earlier CL now, so it should apply.

you're right, you did say that. Sorry for missing it. This patch still doesn't
compile though as you changed the name of the file.

http://codereview.chromium.org/10270031/diff/7002/chrome/browser/resources/ex...
File chrome/browser/resources/extensions/extensions.js (right):

http://codereview.chromium.org/10270031/diff/7002/chrome/browser/resources/ex...
chrome/browser/resources/extensions/extensions.js:23: // See:
http://www.w3.org/TR/2011/WD-html5-20110113/dnd.html#concept-dnd-p
no, you can't access the file names themselves, but you can still determine
whether there are files available (I've just verified this works in practice and
not just in spec). Better to limit the drag effect to file drags only.

http://codereview.chromium.org/10270031/diff/7002/chrome/browser/resources/ex...
chrome/browser/resources/extensions/extensions.js:29: dropTarget.style.opacity =
0;
this is only a problem because you're toggling between display: none and
display: block. (recently display:none things don't like transitioning,
annoyingly)

This also happens to be pretty jarring because it moves everything immediately
rather than smoothly. You will notice that the developer mode controls are
smooth. I think we should emulate this. Only you don't have to worry about
hiding because there's nothing focusable in the drag area (we only have to
display:none the drag controls because they could otherwise be tabbed to).

http://codereview.chromium.org/10270031/diff/7002/chrome/browser/resources/ex...
chrome/browser/resources/extensions/extensions.js:98: if
(document.body.getAttribute('offStoreInstallEnabled') == 'false') {
this also doesn't work for some reason (didn't bother to debug it). You should
use loadTimeData.getBoolean() instead of setting an attribute on the body
element since you don't access it in css either way.

[Aaron Boodman, 2012-05-02 22:45:06]

On Wed, May 2, 2012 at 11:00 AM,  <jam@chromium.org> wrote:
> I was saying get the filename from JS, and verify that the user dragged it
> by
> using ChildProcessSecurityPolicy
>
> RenderViewHostImpl::DragTargetDragEnter calls
> ChildProcessSecurityPolicy::GrantReadFile
>
> but then looking into it deeper, I see now we don't expose
> ChildProcessSecurityPolicy::CanReadFile. it seems if we expose that in the
> interface instead, that's exposing less in the Content API than the
> WebDropData
> object

This is an interesting idea. I looked into it, but there is a major
issue: The full path is not exposed to JavaScript; only the basename.
I can't find a way to get from the JavaScript object to the full path.

- a

[Aaron Boodman, 2012-05-04 02:22:10]

Per direction from UI team, I have made the entire page the drop target. New
screencap here:

http://imgur.com/3EZNc

@jam: per discussion with darin, I've not changed anything on the C++ side.

https://chromiumcodereview.appspot.com/10270031/diff/7002/chrome/browser/reso...
File chrome/browser/resources/extensions/extensions.js (right):

https://chromiumcodereview.appspot.com/10270031/diff/7002/chrome/browser/reso...
chrome/browser/resources/extensions/extensions.js:23: // See:
http://www.w3.org/TR/2011/WD-html5-20110113/dnd.html#concept-dnd-p
On 2012/05/02 22:03:09, Evan Stade wrote:
> no, you can't access the file names themselves, but you can still determine
> whether there are files available (I've just verified this works in practice
and
> not just in spec). Better to limit the drag effect to file drags only.

Okie doke.

https://chromiumcodereview.appspot.com/10270031/diff/7002/chrome/browser/reso...
chrome/browser/resources/extensions/extensions.js:29: dropTarget.style.opacity =
0;
On 2012/05/02 22:03:09, Evan Stade wrote:
> this is only a problem because you're toggling between display: none and
> display: block. (recently display:none things don't like transitioning,
> annoyingly)
> 
> This also happens to be pretty jarring because it moves everything immediately
> rather than smoothly. You will notice that the developer mode controls are
> smooth. I think we should emulate this. Only you don't have to worry about
> hiding because there's nothing focusable in the drag area (we only have to
> display:none the drag controls because they could otherwise be tabbed to).

With the new design this comment is no longer applicable.

https://chromiumcodereview.appspot.com/10270031/diff/7002/chrome/browser/reso...
chrome/browser/resources/extensions/extensions.js:98: if
(document.body.getAttribute('offStoreInstallEnabled') == 'false') {
On 2012/05/02 22:03:09, Evan Stade wrote:
> this also doesn't work for some reason (didn't bother to debug it). You should
> use loadTimeData.getBoolean() instead of setting an attribute on the body
> element since you don't access it in css either way.

Done.

[Evan Stade, 2012-05-04 19:57:17]

lgtm

http://codereview.chromium.org/10270031/diff/29001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extensions.js (right):

http://codereview.chromium.org/10270031/diff/29001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extensions.js:19: shouldAcceptDrag:
function(e) {
the js style checker will probably complain if you don't at least @inheritdoc

[jam, 2012-05-04 20:00:58]

content lgtm