New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 788926 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 780164



Sign in to add a comment

MD Extensions: Dragging an extension with an error doesn't show error dialog

Project Member Reported by hcarmona@chromium.org, Nov 27 2017

Issue description

What steps will reproduce the problem?
1. Drag extension onto chrome://extensions page
2. Get error, click OK

What is the expected result?
Expecting to also get a detailed error with the manifest.json file

What happens instead of that?
Detailed error is only shown when clicking on the "Load Unpacked" button.

See screenshot of missing dialog. Attached a zip w/ an extension that will give an error.
 
Screen Shot 2017-11-27 at 3.26.39 PM.png
100 KB View Download
dogge.zip
24.7 KB Download
Blocking: 780164
Owner: dschuyler@chromium.org
Status: Started (was: Available)
The use of a dialog for these manifest errors (screen shot above) is new to MD extensions. The old UI put the information in the page. With our new design there will/would be one dialog after another, which seems like a poor experience. It's also become more clear to me that we show the same error in two different ways.

Comment 4 by dpa...@chromium.org, Nov 30 2017

Blocking: -780164
Labels: -Pri-1 Pri-2
After discussing with Dave, we don't think this should be a blocker, since it already surfaces the error to the user via the native OS dialog. Un-marking it, and we can keep discussing about the best approach separately from the Stable Launch.
Status: Available (was: Started)
Cc: rdevlin....@chromium.org
Cc: -rdevlin....@chromium.org dschuyler@chromium.org
Labels: Proj-MaterialDesign-WebUI
Owner: rdevlin....@chromium.org
Status: Assigned (was: Available)
Hi Devlin,  
What should happen here? Is it appropriate to mark this WontFix? 
Feel free to set the Owner back to me if you'd like me to make a change.
Hmm... this will be fun.

So, drag-and-drop installation can go through different paths - drag-and-dropping crxs/zips and drag-and-dropping file directories.  The former go through a "packed" installation flow and the latter go through an "unpacked" installation flow.

For the packed flow, we probably don't want to bother showing the specific manifest errors or a retry dialog, because those files aren't easily editable (crx files are also signed).  Just showing a failure dialog is probably fine.  This is similar to what the old page did.  The old page actually showed the load error for zip files, but didn't display the manifest and "Retry" was broken - so let's just not display it.  In this respect, the MD page already works.

Drag-and-dropping a directory will load as unpacked, and this probably *should* show the retry dialog and errors.  The reason it doesn't is because this goes through a separate handler (InstallExtensionHandler in webui rather than the developerPrivate.loadUnpacked() API), and this triggers installation with no response.  The old page would then listen for any and all errors related to manifest loading and display them.  The new page only reacts to failed loads, which is much more suited to the MD page's design (in addition to just being more sane).

I think the right solution here is to update the drag-and-drop handler, at least for unpacked files, to go through the developerPrivate API and use the flows we have already established.  The slightly tricky part here is that, for security reasons, we don't want to just allow the renderer to provide the browser with a path to load, so we get the path through accessing WebContents::GetDropData() and looking at that.  The drop data is cleared by the time the drop happens, so we have to cache it while the drop is happening.  So, we'll at least need some kind of "drop started" API method.

I'll see if I can't take a stab at this.

Comment 9 by dpa...@chromium.org, Dec 13 2017

Status: Started (was: Assigned)
Blocking: 780164
Project Member

Comment 11 by bugdroid1@chromium.org, Dec 14 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f6a37d049830e96463ca4a731cc0697a98beec18

commit f6a37d049830e96463ca4a731cc0697a98beec18
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu Dec 14 18:46:46 2017

[MD Extensions] Support drag-and-drop errors for dropped directories

Support displaying the errors dialog from drag-and-dropped directories.
To do this, notify the developerPrivate API when a drag begins to
cache the directory that is being dragged. Also add an additional
parameter to loadUnpacked, useDraggedPath, to use the cached dragged
path rather than prompting the user to choose a path or using a retryId.
When the user drops the directory, use this to load the extension.
Going through the loadUnpacked method like this has the advantage of
displaying load errors to the user and allowing them to retry.

Note that we cannot simply provide the filepath directly from the JS to
the C++ for security reasons (in the case of a compromised renderer, we
don't want to be able to add an arbitrary extension - this ensures that
the user dragged the extension over the extensions page).

The fact that the drag_and_drop_handler is also used by the non-MD
version complicates this slightly, because we have to work for both
versions of the code. This means there's a bit more mess than I'd
otherwise like. This can be cleaned up when MD extensions launches.

Bug:  788926 

Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I888b2094dcda1e964bca0f2680573387e714ea77
Reviewed-on: https://chromium-review.googlesource.com/809953
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524125}
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/chrome/browser/extensions/api/developer_private/developer_private_api.cc
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/chrome/browser/extensions/api/developer_private/developer_private_api.h
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/chrome/browser/resources/extensions/extensions.js
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/chrome/browser/resources/md_extensions/compiled_resources2.gyp
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/chrome/browser/resources/md_extensions/drag_and_drop_handler.js
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/chrome/browser/resources/md_extensions/drop_overlay.js
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/chrome/browser/resources/md_extensions/manager.html
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/chrome/browser/resources/md_extensions/manager.js
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/chrome/common/extensions/api/developer_private.idl
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/extensions/browser/extension_function_histogram_value.h
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/third_party/closure_compiler/externs/developer_private.js
[modify] https://crrev.com/f6a37d049830e96463ca4a731cc0697a98beec18/tools/metrics/histograms/enums.xml

Labels: Merge-Request-64
Status: Fixed (was: Started)
Project Member

Comment 13 by sheriffbot@chromium.org, Dec 15 2017

Labels: -Merge-Request-64 Hotlist-Merge-Review Merge-Review-64
This bug requires manual review: M64 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-64 Merge-Approved-64
As discussed in meeting, Extensions MD launch is behind a flag. Change has been tested and verified in Canary. Approving merge to M64 branch:3282
Project Member

Comment 15 by bugdroid1@chromium.org, Dec 19 2017

Labels: -merge-approved-64 merge-merged-3282
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ce1ff58265dac42ea587349448f80200d7f98fff

commit ce1ff58265dac42ea587349448f80200d7f98fff
Author: dpapad <dpapad@chromium.org>
Date: Tue Dec 19 02:15:29 2017

[M64 merge][MD Extensions] Support drag-and-drop errors for dropped directories

Support displaying the errors dialog from drag-and-dropped directories.
To do this, notify the developerPrivate API when a drag begins to
cache the directory that is being dragged. Also add an additional
parameter to loadUnpacked, useDraggedPath, to use the cached dragged
path rather than prompting the user to choose a path or using a retryId.
When the user drops the directory, use this to load the extension.
Going through the loadUnpacked method like this has the advantage of
displaying load errors to the user and allowing them to retry.

Note that we cannot simply provide the filepath directly from the JS to
the C++ for security reasons (in the case of a compromised renderer, we
don't want to be able to add an arbitrary extension - this ensures that
the user dragged the extension over the extensions page).

The fact that the drag_and_drop_handler is also used by the non-MD
version complicates this slightly, because we have to work for both
versions of the code. This means there's a bit more mess than I'd
otherwise like. This can be cleaned up when MD extensions launches.

Bug:  788926 

TBR=rdevlin.cronin@chromium.org

(cherry picked from commit f6a37d049830e96463ca4a731cc0697a98beec18)

Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I888b2094dcda1e964bca0f2680573387e714ea77
Reviewed-on: https://chromium-review.googlesource.com/809953
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#524125}
Reviewed-on: https://chromium-review.googlesource.com/833526
Cr-Commit-Position: refs/branch-heads/3282@{#290}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/ce1ff58265dac42ea587349448f80200d7f98fff/chrome/browser/extensions/api/developer_private/developer_private_api.cc
[modify] https://crrev.com/ce1ff58265dac42ea587349448f80200d7f98fff/chrome/browser/extensions/api/developer_private/developer_private_api.h
[modify] https://crrev.com/ce1ff58265dac42ea587349448f80200d7f98fff/chrome/browser/extensions/api/developer_private/developer_private_api_unittest.cc
[modify] https://crrev.com/ce1ff58265dac42ea587349448f80200d7f98fff/chrome/browser/resources/extensions/extensions.js
[modify] https://crrev.com/ce1ff58265dac42ea587349448f80200d7f98fff/chrome/browser/resources/md_extensions/compiled_resources2.gyp
[modify] https://crrev.com/ce1ff58265dac42ea587349448f80200d7f98fff/chrome/browser/resources/md_extensions/drag_and_drop_handler.js
[modify] https://crrev.com/ce1ff58265dac42ea587349448f80200d7f98fff/chrome/browser/resources/md_extensions/drop_overlay.js
[modify] https://crrev.com/ce1ff58265dac42ea587349448f80200d7f98fff/chrome/browser/resources/md_extensions/manager.html
[modify] https://crrev.com/ce1ff58265dac42ea587349448f80200d7f98fff/chrome/browser/resources/md_extensions/manager.js
[modify] https://crrev.com/ce1ff58265dac42ea587349448f80200d7f98fff/chrome/common/extensions/api/developer_private.idl
[modify] https://crrev.com/ce1ff58265dac42ea587349448f80200d7f98fff/third_party/closure_compiler/externs/developer_private.js

Sign in to add a comment