MD Extensions: Dragging an extension with an error doesn't show error dialog |
||||||||||||
Issue descriptionWhat 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.
,
Nov 29 2017
,
Nov 29 2017
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.
,
Nov 30 2017
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.
,
Nov 30 2017
,
Dec 5 2017
,
Dec 5 2017
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.
,
Dec 5 2017
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.
,
Dec 13 2017
,
Dec 13 2017
,
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
,
Dec 15 2017
,
Dec 15 2017
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
,
Dec 18 2017
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
,
Dec 19 2017
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 |
||||||||||||
Comment 1 by hcarmona@chromium.org
, Nov 27 2017