Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2479633004:
Split ShowPage message into two. (Closed)
|
Created:
4 years, 1 month ago by hidehiko Modified:
4 years, 1 month ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, alemate+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, khmel Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSplit ShowPage message into two.
This is preparation to fix the dependency in arc_support_host.
ShowPage is now have two different kinds.
- One for normal page.
- One for error page.
BUG=657687
BUG=b/31079732
TEST=Ran on test device. Ran bots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/dc74e57a2c0f1e7bea8bd858ac7ce5a30809dbf4
Cr-Commit-Position: refs/heads/master@{#430076}
Patch Set 1 #Patch Set 2 : Fix error message #
Total comments: 5
Patch Set 3 : rebase #Patch Set 4 : Address comments. #
Total comments: 1
Patch Set 5 : Fix compile error. #
Messages
Total messages: 24 (14 generated)
Description was changed from ========== Split ShowPage message into two. This is preparation to fix the dependency in arc_support_host. ShowPage is now have two different kinds. - One for normal page. - One for error page. BUG=657687 BUG=b/31079732 TEST=Ran on test device. Ran bots. ========== to ========== Split ShowPage message into two. This is preparation to fix the dependency in arc_support_host. ShowPage is now have two different kinds. - One for normal page. - One for error page. BUG=657687 BUG=b/31079732 TEST=Ran on test device. Ran bots. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hidehiko@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...
hidehiko@chromium.org changed reviewers: + lhchavez@chromium.org, xiyuan@chromium.org
PTAL. This is the preparation to fix the dependency between ArcAuthService and ArcSupportHost. The all-in-one draft CL is here just for reference (it needs to be polished): https://codereview.chromium.org/2472223002/ Thank you for review in advance, - hidehiko https://codereview.chromium.org/2479633004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2479633004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:130: if (page == arc::ArcAuthService::UIPage::ERROR || Note: we'll split ShowPage() into two for the API simplicity. Then, this content will be simplified, too.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm https://codereview.chromium.org/2479633004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2479633004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:49: constexpr char kShowSendFeedback[] = "showSendFeedback"; nit: showSendFeedback -> shouldShowSendFeedback showSendFeedback sounds like an action instead of a boolean.
lgtm https://codereview.chromium.org/2479633004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2479633004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:46: // which is a localized error text, and "showSendFeedback" boolean value. nit: s/an error message/the error page/.
Thank you for review. Submitting. https://codereview.chromium.org/2479633004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2479633004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:46: // which is a localized error text, and "showSendFeedback" boolean value. On 2016/11/04 17:11:25, Luis Héctor Chávez wrote: > nit: s/an error message/the error page/. Done. https://codereview.chromium.org/2479633004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:49: constexpr char kShowSendFeedback[] = "showSendFeedback"; On 2016/11/04 16:52:40, xiyuan wrote: > nit: showSendFeedback -> shouldShowSendFeedback > > showSendFeedback sounds like an action instead of a boolean. Done.
The CQ bit was checked by hidehiko@chromium.org
The CQ bit was unchecked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2479633004/#ps60001 (title: "Address comments.")
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Quick fix for compile error. I think I'm ok to re-submit for this case. https://codereview.chromium.org/2479633004/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2479633004/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:32: constexpr char kStatus[] = "status"; Oops, I didn't hit this locally, and this was hidden by other errors in previous trybot run...
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2479633004/#ps80001 (title: "Fix compile error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Split ShowPage message into two. This is preparation to fix the dependency in arc_support_host. ShowPage is now have two different kinds. - One for normal page. - One for error page. BUG=657687 BUG=b/31079732 TEST=Ran on test device. Ran bots. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Split ShowPage message into two. This is preparation to fix the dependency in arc_support_host. ShowPage is now have two different kinds. - One for normal page. - One for error page. BUG=657687 BUG=b/31079732 TEST=Ran on test device. Ran bots. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/dc74e57a2c0f1e7bea8bd858ac7ce5a30809dbf4 Cr-Commit-Position: refs/heads/master@{#430076} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/dc74e57a2c0f1e7bea8bd858ac7ce5a30809dbf4 Cr-Commit-Position: refs/heads/master@{#430076} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
