Add --use-more-webui runtime flag to toggle WebUI replacements for native dialogs.

Add --use-more-webui runtime flag to toggle WebUI replacements for native dialogs.

This adds the flag --use-more-webui to allow turning on WebUI replacements for native dialogs. This flag is automatically set when --use-pure-views is specified. Modifies replaced native dialogs to be toggled based on flag value. BookmarkEditor has also been refactored to use static constructors to EditDetails to describe what is being edited to make the code more readable.

BUG=96935
TEST=Tested that webui dialogs can be turned on or left off with --use-more-webui.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101581

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101785

[Rick Byers, 2011-08-18 19:25:29]

Looks good!  I especially like how the bookmark editor call sites are simplified
and made much more self-describing.  I have just a few suggestions.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_editor.cc (right):

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.cc:16: // TODO(flackr): Remove this and
create runtime flag for webui dialogs
I'm confused, I thought creating a runtime flag is what this CL is doing?

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.cc:53: if (ChromeWebUI::IsMoreWebUI()
&& (
Minor, but it might be nice to split the WebUI portion here off into a private
ShowWebUI function - just to make it clear that ShowWebUI and ShowNative are on
the same footing and you're just selecting between them here...

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.cc:54: details.type ==
EditDetails::EXISTING_NODE ||
This is (very) temporary, right?  Add a TODO for implementing all types with
WebUI

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.cc:54: details.type ==
EditDetails::EXISTING_NODE ||
Is this indentation level right?  Off the top of my head I would have expected 6
or maybe 4, not 8...

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_editor.h (right):

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.h:32: static EditDetails EditNode(const
BookmarkNode* node);
Nice, I like this change.

Add comments to these methods please.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.h:54: Type type;
Could you make the fields const now (eg. by having the private ctor take all the
field values as arguments)?  If it's easy, it would be nice if this were
immutable IMHO.  Not worth a ton of effort though.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.h:68: EditDetails();
nice

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.h:77: static void
ShowNative(gfx::NativeWindow parent_window,
This can be private now (or soon), right?  It would be really nice if this were
an implementation detail.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.h:84: // manager to add or edit
bookmarks.
add/move the detailed description of the args from above.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7670041/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1501: ChromeWebUI::SetMoreWebUI(
Rather that keep a separate flag which you set at startup, you could just have a
getter that computes the value on demand.  I'm not sure which approach is more
common, but that's what the NTP4 code does, see NewTabUI::NTP4Enabled().  This
startup function is long enough that avoiding adding any code to it might be
nice.  But again, assuming lookup is fast either way, I'd go with whichever
style is more common for runtime flags...

http://codereview.chromium.org/7670041/diff/1/chrome/browser/ui/gtk/certifica...
File chrome/browser/ui/gtk/certificate_viewer.h (left):

http://codereview.chromium.org/7670041/diff/1/chrome/browser/ui/gtk/certifica...
chrome/browser/ui/gtk/certificate_viewer.h:13: void
ShowCertificateViewer(gfx::NativeWindow parent,
Does this header server any purpose now other than including
chrome/browser/certificate_viewer.h?  Could you remove it?

[flackr, 2011-08-23 17:41:43]

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_editor.cc (right):

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.cc:16: // TODO(flackr): Remove this and
create runtime flag for webui dialogs
On 2011/08/18 19:25:30, Rick Byers wrote:
> I'm confused, I thought creating a runtime flag is what this CL is doing?

Oops that was leftover from when I was using --use-pure-views to control this.
Removed.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.cc:53: if (ChromeWebUI::IsMoreWebUI()
&& (
On 2011/08/18 19:25:30, Rick Byers wrote:
> Minor, but it might be nice to split the WebUI portion here off into a private
> ShowWebUI function - just to make it clear that ShowWebUI and ShowNative are
on
> the same footing and you're just selecting between them here...

Done.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.cc:54: details.type ==
EditDetails::EXISTING_NODE ||
On 2011/08/18 19:25:30, Rick Byers wrote:
> Is this indentation level right?  Off the top of my head I would have expected
6
> or maybe 4, not 8...

Done.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.cc:54: details.type ==
EditDetails::EXISTING_NODE ||
On 2011/08/18 19:25:30, Rick Byers wrote:
> This is (very) temporary, right?  Add a TODO for implementing all types with
> WebUI

Done.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
File chrome/browser/bookmarks/bookmark_editor.h (right):

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.h:32: static EditDetails EditNode(const
BookmarkNode* node);
On 2011/08/18 19:25:30, Rick Byers wrote:
> Nice, I like this change.
> 
> Add comments to these methods please.

Done.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.h:54: Type type;
On 2011/08/18 19:25:30, Rick Byers wrote:
> Could you make the fields const now (eg. by having the private ctor take all
the
> field values as arguments)?  If it's easy, it would be nice if this were
> immutable IMHO.  Not worth a ton of effort though.

Done.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.h:77: static void
ShowNative(gfx::NativeWindow parent_window,
On 2011/08/18 19:25:30, Rick Byers wrote:
> This can be private now (or soon), right?  It would be really nice if this
were
> an implementation detail.

Done.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/bookmarks/bookma...
chrome/browser/bookmarks/bookmark_editor.h:84: // manager to add or edit
bookmarks.
On 2011/08/18 19:25:30, Rick Byers wrote:
> add/move the detailed description of the args from above.

Done.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7670041/diff/1/chrome/browser/browser_main.cc#...
chrome/browser/browser_main.cc:1501: ChromeWebUI::SetMoreWebUI(
On 2011/08/18 19:25:30, Rick Byers wrote:
> Rather that keep a separate flag which you set at startup, you could just have
a
> getter that computes the value on demand.  I'm not sure which approach is more
> common, but that's what the NTP4 code does, see NewTabUI::NTP4Enabled().  This
> startup function is long enough that avoiding adding any code to it might be
> nice.  But again, assuming lookup is fast either way, I'd go with whichever
> style is more common for runtime flags...

Done.

http://codereview.chromium.org/7670041/diff/1/chrome/browser/ui/gtk/certifica...
File chrome/browser/ui/gtk/certificate_viewer.h (left):

http://codereview.chromium.org/7670041/diff/1/chrome/browser/ui/gtk/certifica...
chrome/browser/ui/gtk/certificate_viewer.h:13: void
ShowCertificateViewer(gfx::NativeWindow parent,
On 2011/08/18 19:25:30, Rick Byers wrote:
> Does this header server any purpose now other than including
> chrome/browser/certificate_viewer.h?  Could you remove it?

Done.

[Rick Byers, 2011-08-24 18:13:28]

LGTM with one nit.
Also please expand your CL description to include the EditDetails
refactoring/clean-up you did.

http://codereview.chromium.org/7670041/diff/5001/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_editor.h (right):

http://codereview.chromium.org/7670041/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_editor.h:36: // a given parent ndoe.
s/ndoe/node

[flackr, 2011-08-25 15:50:37]

I added to the description to mention the refactoring of BookmarkEditor's
EditDetails constructor.

+arv can you review please?

http://codereview.chromium.org/7670041/diff/5001/chrome/browser/bookmarks/boo...
File chrome/browser/bookmarks/bookmark_editor.h (right):

http://codereview.chromium.org/7670041/diff/5001/chrome/browser/bookmarks/boo...
chrome/browser/bookmarks/bookmark_editor.h:36: // a given parent ndoe.
On 2011/08/24 18:13:28, Rick Byers wrote:
> s/ndoe/node

Done.

[arv (Not doing code reviews), 2011-08-25 16:40:41]

LGTM

My only concern is about those ifdefs and making sure we don't include these
things on other posix platforms.

http://codereview.chromium.org/7670041/diff/9001/chrome/browser/browser_resou...
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/7670041/diff/9001/chrome/browser/browser_resou...
chrome/browser/browser_resources.grd:33: <if expr="is_posix and not is_macosx">
Is it intentional that both CrOS and Linux gets these?

What about Android? It is posix too isn't it?

[flackr, 2011-08-25 18:14:19]

On 2011/08/25 16:40:41, arv wrote:
> LGTM
> 
> My only concern is about those ifdefs and making sure we don't include these
> things on other posix platforms.
> 
>
http://codereview.chromium.org/7670041/diff/9001/chrome/browser/browser_resou...
> File chrome/browser/browser_resources.grd (right):
> 
>
http://codereview.chromium.org/7670041/diff/9001/chrome/browser/browser_resou...
> chrome/browser/browser_resources.grd:33: <if expr="is_posix and not
is_macosx">
> Is it intentional that both CrOS and Linux gets these?
> 
> What about Android? It is posix too isn't it?

The main issue including certificate viewer is that the certificate_model
functions must be implemented.

http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/chrome_common.gypi&exact...

In the 'common_net' target above, these functions are included for anything
posix and not mac, though I'm not sure if there would be a better flag for
inclusion/exclusion.

[flackr, 2011-08-25 20:40:47]

http://codereview.chromium.org/7670041/diff/9001/chrome/browser/browser_resou...
File chrome/browser/browser_resources.grd (right):

http://codereview.chromium.org/7670041/diff/9001/chrome/browser/browser_resou...
chrome/browser/browser_resources.grd:33: <if expr="is_posix and not is_macosx">
On 2011/08/25 16:40:41, arv wrote:
> Is it intentional that both CrOS and Linux gets these?
> 
> What about Android? It is posix too isn't it?

Just to address this question, it is intentional. As long as the
certificate_model functions are supported it shouldn't hurt to have the webui
certificate viewer compiled in. The logic matches that of chrome_common.gypi. Of
course let me know if you think this could be a problem. The main issue was
avoiding including the certificate viewer on windows and mac where because they
used a native certificate viewer the certificate_model functions were not
defined and it would fail in linking.

[flackr, 2011-08-31 17:31:00]

arv, I had to change EditDetails constructor calls in the unit tests too. PTAL,
thanks!

[Rick Byers, 2011-08-31 23:52:46]

On 2011/08/31 17:31:00, flackr wrote:
> arv, I had to change EditDetails constructor calls in the unit tests too.
PTAL,
> thanks!

FWIW LGTM

[commit-bot: I haz the power, 2011-09-01 22:52:59]

Can't apply patch for file chrome/browser/ui/gtk/certificate_viewer.h.
While running svn propset svn:eol-style LF
chrome/browser/ui/gtk/certificate_viewer.h --config-dir
/mnt/data/b/commit-queue/subversion_config --non-interactive;
patching file chrome/browser/ui/gtk/certificate_viewer.h
svn: Can't open file 'chrome/browser/ui/gtk/certificate_viewer.h': No such file
or directory

[M-A Ruel, 2011-09-08 00:16:24]

On 2011/09/01 22:52:59, I haz the power (commit-bot) wrote:
> Can't apply patch for file chrome/browser/ui/gtk/certificate_viewer.h.

I have a fix for this problem, I'll test this patch tomorrow.

[commit-bot: I haz the power, 2011-09-08 20:59:23]

Can't apply patch for file
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
Hunk #1 FAILED at 366.
Hunk #2 succeeded at 388 (offset 3 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc.rej

[M-A Ruel, 2011-09-08 21:00:53]

Sorry I was too slow and bookmark_bubble_view.cc was modified since last time
you uploaded the patch.

Can you sync, resolve conflicts locally, upload again and check the commit box?

Thanks,

M-A

On 2011/09/08 20:59:23, I haz the power (commit-bot) wrote:
> Can't apply patch for file
> chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc.
> While running patch -p1 --forward --force;
> patching file chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
> Hunk #1 FAILED at 366.
> Hunk #2 succeeded at 388 (offset 3 lines).
> 1 out of 2 hunks FAILED -- saving rejects to file
> chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc.rej

[flackr, 2011-09-09 17:58:51]

Merged with trunk.

http://codereview.chromium.org/7670041/diff/32001/chrome/browser/ui/webui/hun...
File chrome/browser/ui/webui/hung_renderer_dialog.cc (right):

http://codereview.chromium.org/7670041/diff/32001/chrome/browser/ui/webui/hun...
chrome/browser/ui/webui/hung_renderer_dialog.cc:101: handler_->CloseDialog();
The close widget method in trunk depended on views which prevented compilation
on linux (and presumably mac). Closing the dialog through javascript will work
on all platforms.

http://codereview.chromium.org/7670041/diff/32001/chrome/browser/ui/webui/hun...
chrome/browser/ui/webui/hung_renderer_dialog.cc:174: string16(),
UTF8ToUTF16("chrome.send('DialogClose');"));
This calls DialogClose with no arguments, which is the right way when no
arguments are required. However ConstrainedHtmlUI currently crashes when
DialogClose receives no arguments. There is a change to fix ConstrainedHtmlUI:
http://codereview.chromium.org/7757006/

[flackr, 2011-09-12 13:58:49]

+arv, rbyers

The merge required some small changes to HungRendererDialog's HideDialog method
to work on non views platforms. It's easiest to see by viewing changes from
trunk. The relevant files to look at are:
  chrome/browser/ui/webui/hung_renderer_dialog.cc
  chrome/browser/ui/webui/hung_renderer_dialog.h

Conflicting files from the merge were:
  chrome/browser/ui/views/bookmarks/bookmark_bubble_view.cc
  chrome/browser/ui/webui/hung_renderer_dialog.cc
  chrome/chrome_browser.gypi

[arv (Not doing code reviews), 2011-09-12 18:34:20]

-1

I revoke my LGTM with the current changes.

http://codereview.chromium.org/7670041/diff/32001/chrome/browser/ui/webui/hun...
File chrome/browser/ui/webui/hung_renderer_dialog.cc (right):

http://codereview.chromium.org/7670041/diff/32001/chrome/browser/ui/webui/hun...
chrome/browser/ui/webui/hung_renderer_dialog.cc:174: string16(),
UTF8ToUTF16("chrome.send('DialogClose');"));
This is really backwards. Why call js just so that the js can call the backend
to do the closing. Please look at how DialogClose is handled and do the same
thing here.

[flackr, 2011-09-12 20:17:19]

http://codereview.chromium.org/7670041/diff/32001/chrome/browser/ui/webui/hun...
File chrome/browser/ui/webui/hung_renderer_dialog.cc (right):

http://codereview.chromium.org/7670041/diff/32001/chrome/browser/ui/webui/hun...
chrome/browser/ui/webui/hung_renderer_dialog.cc:174: string16(),
UTF8ToUTF16("chrome.send('DialogClose');"));
On 2011/09/12 18:34:20, arv wrote:
> This is really backwards. Why call js just so that the js can call the backend
> to do the closing. Please look at how DialogClose is handled and do the same
> thing here.

Done.

[arv (Not doing code reviews), 2011-09-13 02:50:37]

lgtm

[commit-bot: I haz the power, 2011-09-14 13:10:45]

Presubmit check for 7670041-47001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
chrome/browser/sync/test/integration/bookmarks_helper.cc

Presubmit checks took 4.4s to calculate.

[flackr, 2011-09-14 13:21:03]

Adding tim for lgtm on sync change in:
chrome/browser/sync/test/integration/bookmarks_helper.cc

[Rick Campbell, 2011-09-16 17:31:08]

bookmarks_helper.cc LGTM (sync owner)

[commit-bot: I haz the power, 2011-09-16 22:09:32]

Change committed as 101581

[commit-bot: I haz the power, 2011-09-19 18:33:27]

Change committed as 101785

[M-A Ruel, 2011-09-19 18:34:40]

On 2011/09/19 18:33:27, I haz the power (commit-bot) wrote:
> Change committed as 101785

Do not reuse old issues to commit again, please upload to a new issue.

[flackr, 2011-09-19 18:50:05]

On 2011/09/19 18:34:40, Marc-Antoine Ruel wrote:
> On 2011/09/19 18:33:27, I haz the power (commit-bot) wrote:
> > Change committed as 101785
> 
> Do not reuse old issues to commit again, please upload to a new issue.

My apologies, I was told that I could reopen and recommit. Is there any way to
link the new issue to the old one so that reviewers know it is just a minor
change to what they have already reviewed?

[M-A Ruel, 2011-09-19 18:56:53]

On 2011/09/19 18:50:05, flackr wrote:
> My apologies, I was told that I could reopen and recommit. Is there any way to
> link the new issue to the old one so that reviewers know it is just a minor
> change to what they have already reviewed?

The best work flow is:
- Create a new git branch off trunk.
- Revert the revert.
- Upload. This will create the patchset 1 on a new issue with the change as it
was committed.
- Hack, upload again. Then the reviewer can diff against patchset 1.