Fixed chrome://net-export save dialog code

- Removed a DCHECK that prevented "start logging" a second time.
- Parent window is now blocked when the save dialog pops up.
- The SelectFileDialog instance is now cleaned up as soon as the user saves or cancels.

BUG=657219
Committed: https://crrev.com/bfe6949954d4ed616ae69479b17f79142317c722
Cr-Commit-Position: refs/heads/master@{#427422}

[wangyix1, 2016-10-21 21:19:07]

Description was changed from

==========
Debugged the chrome://net-export save dialog code:
- Removed a DCHECK that prevented "start logging" a second time.
- Parent window is now blocked when the save dialog pops up.
- The SelectFileDialog instance is now cleaned up as soon as the user saves or
cancels.

BUG=657219
==========

to

==========
Debugged the chrome://net-export save dialog code:
- Removed a DCHECK that prevented "start logging" a second time.
- Parent window is now blocked when the save dialog pops up.
- The SelectFileDialog instance is now cleaned up as soon as the user saves or
cancels.

BUG=657219
==========

[wangyix1, 2016-10-21 21:23:03]

wangyix@chromium.org changed reviewers:
+ eroman@chromium.org, mmenke@chromium.org, wangyix@chromium.org

[mmenke, 2016-10-24 16:58:59]

https://codereview.chromium.org/2440173003/diff/1/chrome/browser/ui/webui/net...
File chrome/browser/ui/webui/net_export_ui.cc (right):

https://codereview.chromium.org/2440173003/diff/1/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_export_ui.cc:316: // In that case, we don't want
more than one save dialog to appear.
nit:  Don't use we in comments, as it's ambiguous ("We this class", "We the
chrome developers", "We the user", "We the television network", etc).  Maybe
something like "In that case, this is needed to prevent more than one save
dialog from appearing."

https://codereview.chromium.org/2440173003/diff/1/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_export_ui.cc:319: }
nit:  No braces when the if condition and body take up one line each.

https://codereview.chromium.org/2440173003/diff/1/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_export_ui.cc:329: : nullptr;
I don't think webcontents can actually ever be NULL?  Know this is old code, but
looks like the web_contents_ value that web_ui()->GetWebContents() returns is
set on construction of WebUIImpl, and never cleared, so could we remove that
check, while we're here?

https://codereview.chromium.org/2440173003/diff/1/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_export_ui.cc:337: void* params) {
Maybe put a DCHECK(select_file_dialog_); here and in FileSelectionCanceled, for
documentation purposes?

[mmenke, 2016-10-24 17:00:50]

One other thing:  Please update the description.  "Debugged" is the process of a
person identifying and fixing the bug, so it's a bit weird in a description of
what a CL does.  Just using "Fix" instead of "Debugged" should be fine.

[wangyix1, 2016-10-24 19:53:45]

Description was changed from

==========
Debugged the chrome://net-export save dialog code:
- Removed a DCHECK that prevented "start logging" a second time.
- Parent window is now blocked when the save dialog pops up.
- The SelectFileDialog instance is now cleaned up as soon as the user saves or
cancels.

BUG=657219
==========

to

==========
- Removed a DCHECK that prevented "start logging" a second time.
- Parent window is now blocked when the save dialog pops up.
- The SelectFileDialog instance is now cleaned up as soon as the user saves or
cancels.

BUG=657219
==========

[wangyix1, 2016-10-24 19:57:12]

https://codereview.chromium.org/2440173003/diff/1/chrome/browser/ui/webui/net...
File chrome/browser/ui/webui/net_export_ui.cc (right):

https://codereview.chromium.org/2440173003/diff/1/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_export_ui.cc:316: // In that case, we don't want
more than one save dialog to appear.
On 2016/10/24 16:58:58, mmenke wrote:
> nit:  Don't use we in comments, as it's ambiguous ("We this class", "We the
> chrome developers", "We the user", "We the television network", etc).  Maybe
> something like "In that case, this is needed to prevent more than one save
> dialog from appearing."

Done.

https://codereview.chromium.org/2440173003/diff/1/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_export_ui.cc:319: }
On 2016/10/24 16:58:58, mmenke wrote:
> nit:  No braces when the if condition and body take up one line each.

Done.

https://codereview.chromium.org/2440173003/diff/1/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_export_ui.cc:329: : nullptr;
On 2016/10/24 16:58:58, mmenke wrote:
> I don't think webcontents can actually ever be NULL?  Know this is old code,
but
> looks like the web_contents_ value that web_ui()->GetWebContents() returns is
> set on construction of WebUIImpl, and never cleared, so could we remove that
> check, while we're here?

Done.

https://codereview.chromium.org/2440173003/diff/1/chrome/browser/ui/webui/net...
chrome/browser/ui/webui/net_export_ui.cc:337: void* params) {
On 2016/10/24 16:58:58, mmenke wrote:
> Maybe put a DCHECK(select_file_dialog_); here and in FileSelectionCanceled,
for
> documentation purposes?

Done.

[mmenke, 2016-10-24 19:59:07]

LGTM.  Doesn't seem like this is very testable, or I'd ask for tests.

[wangyix1, 2016-10-24 22:12:30]

The CQ bit was checked by wangyix@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-24 22:13:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[wangyix, 2016-10-24 23:53:27]

The CQ bit was unchecked by wangyix@google.com

[wangyix, 2016-10-25 17:01:31]

The CQ bit was checked by wangyix@google.com

[commit-bot: I haz the power, 2016-10-25 17:02:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 17:11:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-25 17:11:15]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mmenke, 2016-10-25 17:26:25]

On 2016/10/25 17:11:15, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

You need someone from chrome/browser/ui/webui/OWNERS to sign off on it, since I
don't own the file.

[wangyix, 2016-10-25 18:06:26]

wangyix@google.com changed reviewers:
+ tommycli@chromium.org

[wangyix, 2016-10-25 18:25:20]

Adding tommycli for OWNERs approval.

[tommycli, 2016-10-25 18:29:05]

On 2016/10/25 18:25:20, wangyix wrote:
> Adding tommycli for OWNERs approval.

RS LGTM

[wangyix, 2016-10-25 18:32:23]

The CQ bit was checked by wangyix@google.com

[commit-bot: I haz the power, 2016-10-25 18:33:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 18:38:49]

Description was changed from

==========
- Removed a DCHECK that prevented "start logging" a second time.
- Parent window is now blocked when the save dialog pops up.
- The SelectFileDialog instance is now cleaned up as soon as the user saves or
cancels.

BUG=657219
==========

to

==========
- Removed a DCHECK that prevented "start logging" a second time.
- Parent window is now blocked when the save dialog pops up.
- The SelectFileDialog instance is now cleaned up as soon as the user saves or
cancels.

BUG=657219
==========

[commit-bot: I haz the power, 2016-10-25 18:38:50]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-25 19:09:07]

Description was changed from

==========
- Removed a DCHECK that prevented "start logging" a second time.
- Parent window is now blocked when the save dialog pops up.
- The SelectFileDialog instance is now cleaned up as soon as the user saves or
cancels.

BUG=657219
==========

to

==========
- Removed a DCHECK that prevented "start logging" a second time.
- Parent window is now blocked when the save dialog pops up.
- The SelectFileDialog instance is now cleaned up as soon as the user saves or
cancels.

BUG=657219

Committed: https://crrev.com/bfe6949954d4ed616ae69479b17f79142317c722
Cr-Commit-Position: refs/heads/master@{#427422}
==========

[commit-bot: I haz the power, 2016-10-25 19:09:08]

Patchset 2 (id:??) landed as
https://crrev.com/bfe6949954d4ed616ae69479b17f79142317c722
Cr-Commit-Position: refs/heads/master@{#427422}

[eroman, 2016-10-25 20:22:58]

LGTM

Note that for CL description, we generally try to structure it as:

   <1 sentence short description>

   <details>


In this manner when running commands like git-log there is a brief one line
summary that appears first.

For future changes I encourage following that pattern.