Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740713002:
Show alert when failed to get token for opening suggested apps dialog. (Closed)
|
Created:
3 years, 9 months ago by yamaguchi Modified:
3 years, 9 months ago Reviewers:
fukino CC:
chromium-reviews, oka+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, yamaguchi+watch_chromium.org, fukino+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow alert when failed to get token for opening suggested apps dialog.
This will fix the regression where opening a ZIP file in the Guest mode
gives no feedback.
BUG=694419
TEST=Manually tested by opening a zip file in Guest mode.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2740713002
Cr-Commit-Position: refs/heads/master@{#455631}
Committed: https://chromium.googlesource.com/chromium/src/+/8cc2674aa0a8f9da76f18fc55bd8adf4a4730a3b
Patch Set 1 #
Total comments: 4
Patch Set 2 : Don't set the private variable as it'd not be read in the code path. #Patch Set 3 : Equally finalize the widget even when token not obtained, to keep the existing behavior. #Messages
Total messages: 22 (14 generated)
Description was changed from ========== Show error alert when access token is not obtainied when suggesting app. This will fix the regression in the Guest mode where opening a ZIP file gives no feedback. BUG=694419 TEST=Manually tested by opening a zip file in Guest mode. ========== to ========== Show error alert when access token is not obtainied when suggesting app. This will fix the regression in the Guest mode where opening a ZIP file gives no feedback. BUG=694419 TEST=Manually tested by opening a zip file in Guest mode. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Show error alert when access token is not obtainied when suggesting app. This will fix the regression in the Guest mode where opening a ZIP file gives no feedback. BUG=694419 TEST=Manually tested by opening a zip file in Guest mode. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Show alert when failed to get token for opening suggested apps dialog. This will fix the regression where opening a ZIP file in the Guest mode gives no feedback. BUG=694419 TEST=Manually tested by opening a zip file in Guest mode. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
yamaguchi@chromium.org changed reviewers: + fukino@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2740713002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js (right): https://codereview.chromium.org/2740713002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:272: this.result_ = SuggestAppsDialog.Result.FAILED; - Do we have to set this.result_ here? I'm not sure where it is used. - Don't we have to call this.widget_.finalizeAndGetResult() as it is done in !dialogShown case?
https://codereview.chromium.org/2740713002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js (right): https://codereview.chromium.org/2740713002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:272: this.result_ = SuggestAppsDialog.Result.FAILED; On 2017/03/08 11:38:41, fukino wrote: > - Do we have to set this.result_ here? I'm not sure where it is used. No. It's read only in onHide_(), called by this.hide(). Thanks for catching. > - Don't we have to call this.widget_.finalizeAndGetResult() as it is done in > !dialogShown case? Based to the doc of the function, I think it should not be called in this case. https://cs.chromium.org/chromium/src/components/chrome_apps/webstore_widget/c... > If called before > * promise returned by {@code this.start} is resolved, the reported result will > * be as if the widget was cancelled. If we call it, It'll increase the count for the metric CWSWidgetContainer.MetricsRecorder.CLOSE_DIALOG.USER_CANCELLED, which I don't think well describes this case.
https://codereview.chromium.org/2740713002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js (right): https://codereview.chromium.org/2740713002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:272: this.result_ = SuggestAppsDialog.Result.FAILED; > > - Don't we have to call this.widget_.finalizeAndGetResult() as it is done in > > !dialogShown case? > > Based to the doc of the function, I think it should not be called in this case. > > https://cs.chromium.org/chromium/src/components/chrome_apps/webstore_widget/c... > > If called before > > * promise returned by {@code this.start} is resolved, the reported result will > > * be as if the widget was cancelled. > > If we call it, It'll increase the count for the metric > CWSWidgetContainer.MetricsRecorder.CLOSE_DIALOG.USER_CANCELLED, which I don't > think well describes this case. But the metric has been recorded before, right? I'm concerned about that the new code introduces the only code path in which the widget container is not reset after showInternal_. If we call hide() or this.widget_.finalizeAndGetResult(), the widget containers internal state will be initialized. But only in this code path, neither will be called. WDYT?
The CQ bit was checked by yamaguchi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2740713002/diff/1/ui/file_manager/file_manage... File ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js (right): https://codereview.chromium.org/2740713002/diff/1/ui/file_manager/file_manage... ui/file_manager/file_manager/foreground/js/ui/suggest_apps_dialog.js:272: this.result_ = SuggestAppsDialog.Result.FAILED; On 2017/03/08 12:51:58, fukino wrote: > > > - Don't we have to call this.widget_.finalizeAndGetResult() as it is done in > > > !dialogShown case? > > > > Based to the doc of the function, I think it should not be called in this > case. > > > > > https://cs.chromium.org/chromium/src/components/chrome_apps/webstore_widget/c... > > > If called before > > > * promise returned by {@code this.start} is resolved, the reported result > will > > > * be as if the widget was cancelled. > > > > If we call it, It'll increase the count for the metric > > CWSWidgetContainer.MetricsRecorder.CLOSE_DIALOG.USER_CANCELLED, which I don't > > think well describes this case. > > But the metric has been recorded before, right? > I'm concerned about that the new code introduces the only code path in which the > widget container is not reset after showInternal_. > If we call hide() or this.widget_.finalizeAndGetResult(), the widget containers > internal state will be initialized. But only in this code path, neither will be > called. > WDYT? I agreed. Since this is a change which will be merged to release branch, we'll keep the change minimal.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks!
The CQ bit was checked by yamaguchi@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489022614272280,
"parent_rev": "1d57799fb3aa77b04cc79982dca0261d6b7d673e", "commit_rev":
"8cc2674aa0a8f9da76f18fc55bd8adf4a4730a3b"}
Message was sent while issue was closed.
Description was changed from ========== Show alert when failed to get token for opening suggested apps dialog. This will fix the regression where opening a ZIP file in the Guest mode gives no feedback. BUG=694419 TEST=Manually tested by opening a zip file in Guest mode. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Show alert when failed to get token for opening suggested apps dialog. This will fix the regression where opening a ZIP file in the Guest mode gives no feedback. BUG=694419 TEST=Manually tested by opening a zip file in Guest mode. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2740713002 Cr-Commit-Position: refs/heads/master@{#455631} Committed: https://chromium.googlesource.com/chromium/src/+/8cc2674aa0a8f9da76f18fc55bd8... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/8cc2674aa0a8f9da76f18fc55bd8... |
