Remove ScopedVector from chrome/browser/ui.

Remove ScopedVector from chrome/browser/ui.

BUG=554289
Committed: https://crrev.com/4c117ad57e4047fb833b02c9ebc5c8aa814598c2
Cr-Commit-Position: refs/heads/master@{#441435}

[Avi (use Gerrit), 2017-01-02 21:50:49]

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

[commit-bot: I haz the power, 2017-01-02 21:50:55]

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

[Avi (use Gerrit), 2017-01-02 21:51:31]

Description was changed from

==========
Remove ScopedVector from chrome/browser/ui.

BUG=554289, 561329
==========

to

==========
Remove ScopedVector from chrome/browser/ui.

BUG=554289
==========

[commit-bot: I haz the power, 2017-01-02 22:07:08]

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

[commit-bot: I haz the power, 2017-01-02 22:07:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-01-02 22:18:52]

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

[commit-bot: I haz the power, 2017-01-02 22:18:58]

Dry run: 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, 2017-01-02 22:34:59]

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

[commit-bot: I haz the power, 2017-01-02 22:35:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-01-02 22:46:32]

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

[commit-bot: I haz the power, 2017-01-02 22:46:39]

Dry run: 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, 2017-01-02 23:00:48]

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

[commit-bot: I haz the power, 2017-01-02 23:00:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-01-02 23:14:25]

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

[commit-bot: I haz the power, 2017-01-02 23:14:35]

Dry run: 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, 2017-01-02 23:31:56]

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

[commit-bot: I haz the power, 2017-01-02 23:31:56]

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-...)

[Avi (use Gerrit), 2017-01-02 23:39:49]

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

[commit-bot: I haz the power, 2017-01-02 23:39:56]

Dry run: 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, 2017-01-02 23:53:26]

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

[commit-bot: I haz the power, 2017-01-02 23:53:27]

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-...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-01-02 23:56:10]

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

[commit-bot: I haz the power, 2017-01-02 23:56:17]

Dry run: 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, 2017-01-03 00:26:40]

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

[commit-bot: I haz the power, 2017-01-03 00:26:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-01-03 00:29:26]

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

[commit-bot: I haz the power, 2017-01-03 00:29:30]

Dry run: 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, 2017-01-03 00:53:56]

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

[commit-bot: I haz the power, 2017-01-03 00:53:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-01-03 00:54:23]

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

[commit-bot: I haz the power, 2017-01-03 00:54:33]

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

[Avi (use Gerrit), 2017-01-03 01:11:40]

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

[commit-bot: I haz the power, 2017-01-03 01:11:44]

Dry run: 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, 2017-01-03 01:36:01]

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

[commit-bot: I haz the power, 2017-01-03 01:36:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-01-03 01:40:29]

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

[commit-bot: I haz the power, 2017-01-03 01:40:41]

Dry run: 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, 2017-01-03 03:23:00]

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

[commit-bot: I haz the power, 2017-01-03 03:23:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-01-03 16:42:02]

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

[commit-bot: I haz the power, 2017-01-03 16:42:12]

Dry run: 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, 2017-01-03 17:38:20]

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

[commit-bot: I haz the power, 2017-01-03 17:38:21]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-01-03 17:39:31]

avi@chromium.org changed reviewers:
+ thakis@chromium.org

[Avi (use Gerrit), 2017-01-03 17:39:32]

Nico, ptal.

[Avi (use Gerrit), 2017-01-03 17:41:49]

avi@chromium.org changed reviewers:
+ bauerb@chromium.org

[Avi (use Gerrit), 2017-01-03 17:41:50]

WebUI to bauerb

[Nico, 2017-01-03 18:12:35]

nits galore, else lgtm

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm
(right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm:88:
bridges_.erase(it);
nit: fwiw I find

  for (const auto& it = bridges_.begin(); it != bridges_.end(); it++) {
    if (*it == bridge) {
      bridges.erase(it);
      return;
    }
  }

shorter / easier to understand / etc than the fancypants version here

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:220: size_t popped_out_index =
popped_out_it - toolbar_actions_.begin();
again,

  size_t popped_out_index = 0;
  for (; popped_out_index < toolbar_actions_.size(); ++popped_out_index) {
    if (toolbar_actions_[i] == popped_out_action_)
      break;
  }

is half the code

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:498: return index <
GetIconCount() || action == popped_out_action_;
nit: I'd compare action to popped_out_action_ first and only walk the whole
array if it isn't found. And again

  for (size_t i = 0; i < std::min(toolbar_actions_.size(), GetIconCount()); ++i)
{
    if (toolbar_actions_[i] == action)
      return true;
  }
  return false;

for the loop seems simpler to me.

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:745: });
guess :-)

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc (right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc:41: const auto&
toolbar_actions = actions_bar->toolbar_actions_unordered();
i think style guide says that this should have an explicit type

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/hung_renderer_view.cc:143: });
again, just use a loop instead of find_if and then foo - bar.begin()

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h
(right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h:48:
std::vector<PermissionRequest*> requests() {
probably now big enough to be out-of-line?

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/identity_internals_ui.cc (right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/identity_internals_ui.cc:168: });
same

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:1195:
std::vector<std::unique_ptr<base::Value>> args;
Hey, looks like http://chromium-cpp.appspot.com/ now allows initializer lists.
…ooh, but they don't work with move-only types, do they. never mind.

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:1205:
arg_ptrs.push_back(arg.get());
Is there any value in having args be vector? Maybe

  auto title_value = base::MakeUnique<base::StringValue>(title);
  auto error_value = base::MakeUnique<base::StringValue>(error);
  // ...
  std::vector<const base::Value*> args = {title_value.get(), error_value.get(),
...};
  web_ui()->CallJFU("...", args)

is shorter / simpler? (up to you)

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/preferences_browsertest.cc (right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/preferences_browsertest.cc:506:
pref_service()->GetDefaultPrefValue(name->c_str())->CreateDeepCopy());
The reasoning being "eh it's just a test" I assume?

[Avi (use Gerrit), 2017-01-03 21:43:21]

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

[commit-bot: I haz the power, 2017-01-03 21:43:52]

Dry run: 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, 2017-01-03 22:00:08]

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

[commit-bot: I haz the power, 2017-01-03 22:00:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-01-03 22:14:32]

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

[commit-bot: I haz the power, 2017-01-03 22:15:16]

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

[Avi (use Gerrit), 2017-01-03 22:54:54]

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm
(right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/passwords/account_avatar_fetcher_manager.mm:88:
bridges_.erase(it);
On 2017/01/03 18:12:35, Nico (ooo sick) wrote:
> nit: fwiw I find
> 
>   for (const auto& it = bridges_.begin(); it != bridges_.end(); it++) {
>     if (*it == bridge) {
>       bridges.erase(it);
>       return;
>     }
>   }
> 
> shorter / easier to understand / etc than the fancypants version here

Done.

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:220: size_t popped_out_index =
popped_out_it - toolbar_actions_.begin();
On 2017/01/03 18:12:35, Nico (ooo sick) wrote:
> again,
> 
>   size_t popped_out_index = 0;
>   for (; popped_out_index < toolbar_actions_.size(); ++popped_out_index) {
>     if (toolbar_actions_[i] == popped_out_action_)
>       break;
>   }
> 
> is half the code

Done.

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:498: return index <
GetIconCount() || action == popped_out_action_;
On 2017/01/03 18:12:35, Nico (ooo sick) wrote:
> nit: I'd compare action to popped_out_action_ first and only walk the whole
> array if it isn't found. And again
> 
>   for (size_t i = 0; i < std::min(toolbar_actions_.size(), GetIconCount());
++i)
> {
>     if (toolbar_actions_[i] == action)
>       return true;
>   }
>   return false;
> 
> for the loop seems simpler to me.

Done.

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:745: });
On 2017/01/03 18:12:35, Nico (ooo sick) wrote:
> guess :-)

Done.

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc (right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc:41: const auto&
toolbar_actions = actions_bar->toolbar_actions_unordered();
On 2017/01/03 18:12:35, Nico (ooo sick) wrote:
> i think style guide says that this should have an explicit type

Done.

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/view...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/view...
chrome/browser/ui/views/hung_renderer_view.cc:143: });
On 2017/01/03 18:12:35, Nico (ooo sick) wrote:
> again, just use a loop instead of find_if and then foo - bar.begin()

Done.

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webs...
File chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h
(right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webs...
chrome/browser/ui/website_settings/permission_bubble_browser_test_util.h:48:
std::vector<PermissionRequest*> requests() {
On 2017/01/03 18:12:35, Nico (ooo sick) wrote:
> probably now big enough to be out-of-line?

Done.

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/identity_internals_ui.cc (right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/identity_internals_ui.cc:168: });
On 2017/01/03 18:12:35, Nico (ooo sick) wrote:
> same

Done.

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:1195:
std::vector<std::unique_ptr<base::Value>> args;
On 2017/01/03 18:12:35, Nico (ooo sick) wrote:
> Hey, looks like http://chromium-cpp.appspot.com/ now allows initializer lists.
> …ooh, but they don't work with move-only types, do they. never mind.

:(

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:1205:
arg_ptrs.push_back(arg.get());
On 2017/01/03 18:12:35, Nico (ooo sick) wrote:
> Is there any value in having args be vector? Maybe
> 
>   auto title_value = base::MakeUnique<base::StringValue>(title);
>   auto error_value = base::MakeUnique<base::StringValue>(error);
>   // ...
>   std::vector<const base::Value*> args = {title_value.get(),
error_value.get(),
> ...};
>   web_ui()->CallJFU("...", args)
> 
> is shorter / simpler? (up to you)

Done.

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/preferences_browsertest.cc (right):

https://codereview.chromium.org/2606293002/diff/200001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/preferences_browsertest.cc:506:
pref_service()->GetDefaultPrefValue(name->c_str())->CreateDeepCopy());
On 2017/01/03 18:12:35, Nico (ooo sick) wrote:
> The reasoning being "eh it's just a test" I assume?

I don't know what the ownership story of GetDefaultPrefValue is. The old code
makes a copy, so I'm assuming a copy is needed.

[commit-bot: I haz the power, 2017-01-03 23:08:16]

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

[commit-bot: I haz the power, 2017-01-03 23:08:17]

Dry run: This issue passed the CQ dry run.

[Nico, 2017-01-03 23:15:44]

still lgtm

https://codereview.chromium.org/2606293002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://codereview.chromium.org/2606293002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/hung_renderer_view.cc:145: int index =
static_cast<int>(i - tab_observers_.begin());
i meant this subtraction is kind of silly; if you looped over an int instead of
an iterator you wouldn't need this. (this is a vector after all)

size_t index = 0;
for (; index < tab_observers.size(); ++index)
  if tab_observers[index].get() == tab
    break
DCHECK(i < tab_observers.size());

[Avi (use Gerrit), 2017-01-04 00:03:45]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2017-01-04 00:03:45]

The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2606293002/#ps260001
(title: "one last")

[commit-bot: I haz the power, 2017-01-04 00:04:19]

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

[Avi (use Gerrit), 2017-01-04 00:04:50]

https://codereview.chromium.org/2606293002/diff/240001/chrome/browser/ui/view...
File chrome/browser/ui/views/hung_renderer_view.cc (right):

https://codereview.chromium.org/2606293002/diff/240001/chrome/browser/ui/view...
chrome/browser/ui/views/hung_renderer_view.cc:145: int index =
static_cast<int>(i - tab_observers_.begin());
On 2017/01/03 23:15:44, Nico (ooo sick) wrote:
> i meant this subtraction is kind of silly; if you looped over an int instead
of
> an iterator you wouldn't need this. (this is a vector after all)
> 
> size_t index = 0;
> for (; index < tab_observers.size(); ++index)
>   if tab_observers[index].get() == tab
>     break
> DCHECK(i < tab_observers.size());

Done.

[commit-bot: I haz the power, 2017-01-04 00:13:51]

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

[commit-bot: I haz the power, 2017-01-04 00:13:52]

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...)

[Bernhard Bauer, 2017-01-04 11:20:09]

lgtm

https://codereview.chromium.org/2606293002/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/identity_internals_ui.cc (right):

https://codereview.chromium.org/2606293002/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/identity_internals_ui.cc:169:
token_revokers_.erase(iter);
Can you move this inside the loop and then make it a regular for (auto it :
token_revokers_) loop? You wouldn't get the DCHECK, but given that that's it's
by definition not required to execute the DCHECK, I'd be okay with leaving it
out (or maybe using a boolean flag |found|)?

[Avi (use Gerrit), 2017-01-04 16:25:08]

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

[commit-bot: I haz the power, 2017-01-04 16:25:23]

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

[Avi (use Gerrit), 2017-01-04 16:42:34]

https://codereview.chromium.org/2606293002/diff/260001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/identity_internals_ui.cc (right):

https://codereview.chromium.org/2606293002/diff/260001/chrome/browser/ui/webu...
chrome/browser/ui/webui/identity_internals_ui.cc:169:
token_revokers_.erase(iter);
On 2017/01/04 11:20:09, Bernhard Bauer wrote:
> Can you move this inside the loop and then make it a regular for (auto it :
> token_revokers_) loop? You wouldn't get the DCHECK, but given that that's it's
> by definition not required to execute the DCHECK, I'd be okay with leaving it
> out (or maybe using a boolean flag |found|)?

I can't switch to an object iterator because I can't then delete, but I can do
something close to that.

[Avi (use Gerrit), 2017-01-04 16:43:14]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2017-01-04 16:43:14]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2606293002/#ps300001
(title: "one last fix")

[commit-bot: I haz the power, 2017-01-04 16:43:32]

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, 2017-01-04 16:50:27]

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

[commit-bot: I haz the power, 2017-01-04 16:50:29]

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...)

[Avi (use Gerrit), 2017-01-04 17:30:57]

avi@chromium.org changed reviewers:
+ dbeam@chromium.org, stevenjb@chromium.org

[Avi (use Gerrit), 2017-01-04 17:30:58]

Oh.

Dan, Steven: can you look at chrome/browser/ui/web_ui/options?

[stevenjb, 2017-01-04 17:56:16]

lgtm with one suggestion that can be ignored or could be done as a cleanup pass,
and some nits that can also be ignored.

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:1206:
ok_callback_value.get(), cancel_callback_value.get()};
This all seems awkward. Would it make sense to add a version of
CallJavascriptFunctionUnsafe that takes a ListValue*? That would be a lot
simpler:

auto args = base::MakeUnique<base::ListValue>();
args->AppendString(title);
...

Also, this formatting looks odd, did clang do this?

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/preferences_browsertest.cc (right):

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/preferences_browsertest.cc:355: for (auto name =
pref_names.begin(); name != pref_names.end(); ++name) {
nit: const auto&

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/preferences_browsertest.cc:497:
std::unique_ptr<base::ListValue> list = base::MakeUnique<base::ListValue>();
nit: auto

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/preferences_browsertest.cc:504: for (auto name =
pref_names_.begin(); name != pref_names_.end(); ++name) {
const&

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/preferences_browsertest.cc:970:
std::unique_ptr<base::ListValue> list = base::MakeUnique<base::ListValue>();
auto

[Avi (use Gerrit), 2017-01-04 18:44:40]

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:1206:
ok_callback_value.get(), cancel_callback_value.get()};
On 2017/01/04 17:56:15, stevenjb wrote:
> This all seems awkward. Would it make sense to add a version of
> CallJavascriptFunctionUnsafe that takes a ListValue*?

We can't overload just the ListValue, as calling a Javascript function with a
single list parameter is valid. The interface is structured so that the vector
side is implying a list, but a list of arguments...

OTOH, this seems to be designed to encourage the use of ScopedVector.

Let me follow up with a better design if I can find one.

> Also, this formatting looks odd, did clang do this?

Yep, entirely its doing.

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/options/preferences_browsertest.cc (right):

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/preferences_browsertest.cc:355: for (auto name =
pref_names.begin(); name != pref_names.end(); ++name) {
On 2017/01/04 17:56:16, stevenjb wrote:
> nit: const auto&

Done.

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/preferences_browsertest.cc:497:
std::unique_ptr<base::ListValue> list = base::MakeUnique<base::ListValue>();
On 2017/01/04 17:56:16, stevenjb wrote:
> nit: auto

Done here and elsewhere.

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/preferences_browsertest.cc:504: for (auto name =
pref_names_.begin(); name != pref_names_.end(); ++name) {
On 2017/01/04 17:56:16, stevenjb wrote:
> const&

Done.

https://codereview.chromium.org/2606293002/diff/300001/chrome/browser/ui/webu...
chrome/browser/ui/webui/options/preferences_browsertest.cc:970:
std::unique_ptr<base::ListValue> list = base::MakeUnique<base::ListValue>();
On 2017/01/04 17:56:16, stevenjb wrote:
> auto

Done.

[Avi (use Gerrit), 2017-01-04 18:47:04]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2017-01-04 18:47:05]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
stevenjb@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2606293002/#ps320001
(title: "nits")

[commit-bot: I haz the power, 2017-01-04 18:47:34]

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, 2017-01-04 19:28:52]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1483555624953290,
"parent_rev": "784be8929372b9a667f72eb33eaa2d56486e3dbc", "commit_rev":
"61bdba81ef8289ace5a95d6df74c1453170bb817"}

[commit-bot: I haz the power, 2017-01-04 19:29:44]

Description was changed from

==========
Remove ScopedVector from chrome/browser/ui.

BUG=554289
==========

to

==========
Remove ScopedVector from chrome/browser/ui.

BUG=554289

Review-Url: https://codereview.chromium.org/2606293002
==========

[commit-bot: I haz the power, 2017-01-04 19:29:49]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2017-01-04 19:33:36]

Description was changed from

==========
Remove ScopedVector from chrome/browser/ui.

BUG=554289

Review-Url: https://codereview.chromium.org/2606293002
==========

to

==========
Remove ScopedVector from chrome/browser/ui.

BUG=554289

Committed: https://crrev.com/4c117ad57e4047fb833b02c9ebc5c8aa814598c2
Cr-Commit-Position: refs/heads/master@{#441435}
==========

[commit-bot: I haz the power, 2017-01-04 19:33:38]

Patchset 17 (id:??) landed as
https://crrev.com/4c117ad57e4047fb833b02c9ebc5c8aa814598c2
Cr-Commit-Position: refs/heads/master@{#441435}