Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
236203017:
Fixes the crash for "learn more" link of the dialog for v2 apps. (Closed)
|
Created:
6 years, 8 months ago by Jun Mukai Modified:
6 years, 8 months ago CC:
chromium-reviews, nkostylev+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, dkrahn+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFixes the crash for "learn more" link of the dialog for v2 apps.
It assumed a browser but it might not exist in case of v2 apps.
BUG=363716
R=dkrahn@chromium.org, bartfab@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264451
Patch Set 1 #
Total comments: 18
Patch Set 2 : fix #
Messages
Total messages: 15 (0 generated)
On 2014/04/16 00:21:41, Jun Mukai wrote: [non-committer] I'm not deeply familiar with the details here but lgtm code-wise.
+bartfab, could you take a look?
https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:8: #include "chrome/browser/ui/browser_finder.h" Nit: No longer used. https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:133: GURL learn_more_url(chrome::kEnhancedPlaybackNotificationLearnMoreURL); Nit: const. https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:135: // |web_contents_| might not have a browser in case of v2 apps, in that case Nit: s/, in that case/. In that case,/ https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:136: // open a new tab in a usual way. 1: Nit: s/ a / the / 2: How do you open a new tab when there is no browser? Is it not that you actually open a whole new browser, which then contains one tab? https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:138: Profile* profile = Profile::FromBrowserContext( Nit: #include "chrome/browser/profiles/profile.h" https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:141: profile, learn_more_url, content::PAGE_TRANSITION_LINK); Nit: #include "content/public/common/page_transition_types.h" https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:144: chrome::ShowSingletonTab(browser, GURL( Nit: Use the |learn_more_url| constructed above instead of reconstructing it here.
https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:8: #include "chrome/browser/ui/browser_finder.h" On 2014/04/16 12:22:42, bartfab wrote: > Nit: No longer used. browser_finder is still used in line 132 below https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:133: GURL learn_more_url(chrome::kEnhancedPlaybackNotificationLearnMoreURL); On 2014/04/16 12:22:42, bartfab wrote: > Nit: const. Done. https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:135: // |web_contents_| might not have a browser in case of v2 apps, in that case On 2014/04/16 12:22:42, bartfab wrote: > Nit: s/, in that case/. In that case,/ Done. https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:136: // open a new tab in a usual way. On 2014/04/16 12:22:42, bartfab wrote: > 1: Nit: s/ a / the / > 2: How do you open a new tab when there is no browser? Is it not that you > actually open a whole new browser, which then contains one tab? 1: done. 2: as far as I see browser_navigator.cc, it will open a new browser window with a new tab, or use a new tab in an existing browser window if exists. Note that this is not the case of 'no browser window'. This is 'web_contents is not in a browser window'. There might be other browser windows. Do you feel to describe whole things? https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:138: Profile* profile = Profile::FromBrowserContext( On 2014/04/16 12:22:42, bartfab wrote: > Nit: #include "chrome/browser/profiles/profile.h" Done. https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:141: profile, learn_more_url, content::PAGE_TRANSITION_LINK); On 2014/04/16 12:22:42, bartfab wrote: > Nit: #include "content/public/common/page_transition_types.h" Done. https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:141: profile, learn_more_url, content::PAGE_TRANSITION_LINK); On 2014/04/16 12:22:42, bartfab wrote: > Nit: #include "content/public/common/page_transition_types.h" Done. https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:144: chrome::ShowSingletonTab(browser, GURL( On 2014/04/16 12:22:42, bartfab wrote: > Nit: Use the |learn_more_url| constructed above instead of reconstructing it > here. Done.
lgtm https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:136: // open a new tab in a usual way. On 2014/04/16 18:00:59, Jun Mukai wrote: > On 2014/04/16 12:22:42, bartfab wrote: > > 1: Nit: s/ a / the / > > 2: How do you open a new tab when there is no browser? Is it not that you > > actually open a whole new browser, which then contains one tab? > > 1: done. > > 2: as far as I see browser_navigator.cc, it will open a new browser window with > a new tab, or use a new tab in an existing browser window if exists. Note that > this is not the case of 'no browser window'. This is 'web_contents is not in a > browser window'. There might be other browser windows. > Do you feel to describe whole things? Thanks for clarifying. I am so used to Chrome OS single-app kiosk mode that whenever I hear app v2, I think "app that owns the entire screen - no other browser windows." That confused me here. I think there is no need to add any comments - all confusion has been addressed.
https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:136: // open a new tab in a usual way. On 2014/04/16 18:38:59, bartfab wrote: > On 2014/04/16 18:00:59, Jun Mukai wrote: > > On 2014/04/16 12:22:42, bartfab wrote: > > > 1: Nit: s/ a / the / > > > 2: How do you open a new tab when there is no browser? Is it not that you > > > actually open a whole new browser, which then contains one tab? > > > > 1: done. > > > > 2: as far as I see browser_navigator.cc, it will open a new browser window > with > > a new tab, or use a new tab in an existing browser window if exists. Note > that > > this is not the case of 'no browser window'. This is 'web_contents is not in > a > > browser window'. There might be other browser windows. > > Do you feel to describe whole things? > > Thanks for clarifying. I am so used to Chrome OS single-app kiosk mode that > whenever I hear app v2, I think "app that owns the entire screen - no other > browser windows." That confused me here. I think there is no need to add any > comments - all confusion has been addressed. The report is saying a normal v2 app in a normal chromebook, but certainly there are no browser windows in the Kiosk mode. I'll double-check with the PM but I think this dialog is not used in Kiosk mode.
https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... File chrome/browser/chromeos/attestation/platform_verification_dialog.cc (right): https://codereview.chromium.org/236203017/diff/1/chrome/browser/chromeos/atte... chrome/browser/chromeos/attestation/platform_verification_dialog.cc:136: // open a new tab in a usual way. On 2014/04/16 21:23:41, Jun Mukai wrote: > On 2014/04/16 18:38:59, bartfab wrote: > > On 2014/04/16 18:00:59, Jun Mukai wrote: > > > On 2014/04/16 12:22:42, bartfab wrote: > > > > 1: Nit: s/ a / the / > > > > 2: How do you open a new tab when there is no browser? Is it not that you > > > > actually open a whole new browser, which then contains one tab? > > > > > > 1: done. > > > > > > 2: as far as I see browser_navigator.cc, it will open a new browser window > > with > > > a new tab, or use a new tab in an existing browser window if exists. Note > > that > > > this is not the case of 'no browser window'. This is 'web_contents is not > in > > a > > > browser window'. There might be other browser windows. > > > Do you feel to describe whole things? > > > > Thanks for clarifying. I am so used to Chrome OS single-app kiosk mode that > > whenever I hear app v2, I think "app that owns the entire screen - no other > > browser windows." That confused me here. I think there is no need to add any > > comments - all confusion has been addressed. > > The report is saying a normal v2 app in a normal chromebook, but certainly there > are no browser windows in the Kiosk mode. > I'll double-check with the PM but I think this dialog is not used in Kiosk mode. we do not have any plan to provide this in Kiosk apps so I'll put this into CQ.
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/236203017/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
The CQ bit was checked by mukai@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/236203017/20001
Message was sent while issue was closed.
Change committed as 264451 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
