Switch on use_new_wrapper_types mode for content/common.

Switch on use_new_wrapper_types mode for content/common.

Uses STL vector/string types instead of mojo::{Array,String}
for all mojoms in content/common.
  content/common/frame.mojom
  content/common/render_frame_message_filter.mojom
  content/common/render_widget_window_tree_client_factory.mojom
  content/common/service_worker/embedded_worker_setup.mojom
  content/common/storage_partition_service.mojom
  content/common/leveldb_wrapper.mojom
  content/common/image_downloader/image_downloader.mojom
  content/common/websocket.mojom

BUG=624136
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/c222853dd0d023e96fd00daabb6e610069dc606d
Cr-Commit-Position: refs/heads/master@{#412176}

[leonhsl(Using Gerrit), 2016-08-01 07:39:25]

Description was changed from

==========
Switch on use_new_wrapper_types mode for content/common.

Uses STL vector/string types instead of mojo::{Array,String}
for all mojoms in content/common.
  content/common/frame.mojom
  content/common/render_frame_message_filter.mojom
  content/common/render_widget_window_tree_client_factory.mojom
  content/common/service_worker/embedded_worker_setup.mojom
  content/common/storage_partition_service.mojom
  content/common/leveldb_wrapper.mojom
  content/common/image_downloader/image_downloader.mojom
  content/common/process_control.mojom

BUG=624136
==========

to

==========
Switch on use_new_wrapper_types mode for content/common.

Uses STL vector/string types instead of mojo::{Array,String}
for all mojoms in content/common.
  content/common/frame.mojom
  content/common/render_frame_message_filter.mojom
  content/common/render_widget_window_tree_client_factory.mojom
  content/common/service_worker/embedded_worker_setup.mojom
  content/common/storage_partition_service.mojom
  content/common/leveldb_wrapper.mojom
  content/common/image_downloader/image_downloader.mojom
  content/common/process_control.mojom

BUG=624136
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[leonhsl(Using Gerrit), 2016-08-01 07:42:19]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 07:42:29]

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, 2016-08-01 09:19:20]

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

[commit-bot: I haz the power, 2016-08-01 09:19:20]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-08-01 09:37:28]

leon.han@intel.com changed reviewers:
+ jam@chromium.org, michaeln@chromium.org, thestig@chromium.org,
yzshen@chromium.org

[leonhsl(Using Gerrit), 2016-08-01 09:37:28]

Hi, would you PTAL at this? Thanks.

Yuzhu: For review of everything

michaeln@: For review of leveldb_wrapper_impl.cc and
local_storage_cached_area.cc

jam@: For review of content/

thestig@: For review of base/strings/

[Lei Zhang, 2016-08-01 17:08:42]

On 2016/08/01 09:37:28, leonhsl wrote:
> thestig@: For review of base/strings/

I'm not sure we want everything that one can do with strings in string_util.h.
Can you justify the need to put it in base? Can you leave
Uint8VectorToString16() in local_storage_cached_area.cc instead, for instance?

[michaeln, 2016-08-02 00:28:30]

https://codereview.chromium.org/2201763002/diff/1/content/browser/leveldb_wra...
File content/browser/leveldb_wrapper_impl.cc (right):

https://codereview.chromium.org/2201763002/diff/1/content/browser/leveldb_wra...
content/browser/leveldb_wrapper_impl.cc:105: old_value.swap(*(found->second));
Does this mutate the map? I think it clears out the entry for key? Looks like
the early return on line 116 will have cleared out the old value without storing
the new value.

https://codereview.chromium.org/2201763002/diff/1/content/browser/leveldb_wra...
content/browser/leveldb_wrapper_impl.cc:114: if (new_item_size > old_item_size
&& new_bytes_used > max_size_) {
one option could be could to swap the old_value back into the map_ here

https://codereview.chromium.org/2201763002/diff/1/content/browser/leveldb_wra...
content/browser/leveldb_wrapper_impl.cc:333: if (!it.second) {
ah, i guess this is why the value is optional, to distinguish between puts and
deletes

https://codereview.chromium.org/2201763002/diff/1/content/browser/leveldb_wra...
File content/browser/leveldb_wrapper_impl.h (right):

https://codereview.chromium.org/2201763002/diff/1/content/browser/leveldb_wra...
content/browser/leveldb_wrapper_impl.h:57: std::map<std::vector<uint8_t>,
base::Optional<std::vector<uint8_t>>>;
why is the value type optional?

[leonhsl(Using Gerrit), 2016-08-02 03:10:02]

On 2016/08/01 17:08:42, Lei Zhang wrote:
> On 2016/08/01 09:37:28, leonhsl wrote:
> > thestig@: For review of base/strings/
> 
> I'm not sure we want everything that one can do with strings in string_util.h.
> Can you justify the need to put it in base? Can you leave
> Uint8VectorToString16() in local_storage_cached_area.cc instead, for instance?

I supposed that these conversion functions could be some utils so I took a try
to put them into base/ ;-) OK, I'll put them into local_storage_cached_area.cc
instead. Thanks a lot for confirmation.

[leonhsl(Using Gerrit), 2016-08-02 11:06:51]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-02 11:07:00]

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

[leonhsl(Using Gerrit), 2016-08-02 11:11:05]

Uploaded ps#2:
  -- Moved Uint8VectorToString16 and String16ToUint8Vector impl into
local_storage_cached_area.cc.
  -- Addressed Michael's comments in leveldb_wrapper_impl.cc.

Would you PTAnL? Thanks~

https://codereview.chromium.org/2201763002/diff/1/content/browser/leveldb_wra...
File content/browser/leveldb_wrapper_impl.cc (right):

https://codereview.chromium.org/2201763002/diff/1/content/browser/leveldb_wra...
content/browser/leveldb_wrapper_impl.cc:105: old_value.swap(*(found->second));
On 2016/08/02 00:28:29, michaeln wrote:
> Does this mutate the map? I think it clears out the entry for key? Looks like
> the early return on line 116 will have cleared out the old value without
storing
> the new value.

Yeah, this should be a problem.. Maybe we can move old_value
definition/assignment before line 124? I did so in ps#2. WDYT? Thanks.

https://codereview.chromium.org/2201763002/diff/1/content/browser/leveldb_wra...
content/browser/leveldb_wrapper_impl.cc:333: if (!it.second) {
On 2016/08/02 00:28:29, michaeln wrote:
> ah, i guess this is why the value is optional, to distinguish between puts and
> deletes

Yeah ;-) while mojo::Array has null state inside but std::vector has no, so we
need to use base::optional to enable represent a null value which means a
delete.

https://codereview.chromium.org/2201763002/diff/1/content/browser/leveldb_wra...
File content/browser/leveldb_wrapper_impl.h (right):

https://codereview.chromium.org/2201763002/diff/1/content/browser/leveldb_wra...
content/browser/leveldb_wrapper_impl.h:57: std::map<std::vector<uint8_t>,
base::Optional<std::vector<uint8_t>>>;
On 2016/08/02 00:28:29, michaeln wrote:
> why is the value type optional?

Yeah, it is to distinguish between puts and deletes, null value means a delete.

[commit-bot: I haz the power, 2016-08-02 12:11:12]

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

[commit-bot: I haz the power, 2016-08-02 12:11:13]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-08-02 16:20:09]

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
File content/browser/leveldb_wrapper_impl.cc (right):

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:43: count += (pair.first.size(),
pair.second.value().size());
pair.second may be null, right? (e.g., see line 161) It seems you cannot
de-reference it directly in that case.

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:100: if (*(found->second) == value) {
It seems here we also assume that found->second is not null. Is that intended?

(And also line 164, 210, 226)

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:104: old_item_size = key.size() +
found->second->size();
Previously the old value was removed from the map, but the new code doesn't. Is
that intended? The code may return on line 114. In that case the contents of
|map_| will be different before/after the change.

[michaeln, 2016-08-02 20:31:54]

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
File content/browser/leveldb_wrapper_impl.cc (right):

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:43: count += (pair.first.size(),
pair.second.value().size());
On 2016/08/02 16:20:09, yzshen1 wrote:
> pair.second may be null, right? (e.g., see line 161) It seems you cannot
> de-reference it directly in that case.

also, this line looks weird, can you fix this to clearly add both size values

 count += first.size;
 if (second)
   count += second.value().size;

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:100: if (*(found->second) == value) {
On 2016/08/02 16:20:09, yzshen1 wrote:
> It seems here we also assume that found->second is not null. Is that intended?
> 
> (And also line 164, 210, 226)

Values in this map_ may not be null, it might be good to DCHECK() that under
line 99.

Values in the CommitBatch's map may be null, thats how deletes are represented.

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:104: old_item_size = key.size() +
found->second->size();
On 2016/08/02 16:20:09, yzshen1 wrote:
> Previously the old value was removed from the map, but the new code doesn't.
Is
> that intended? The code may return on line 114. In that case the contents of
> |map_| will be different before/after the change.

yes, this fixes a bug

[leonhsl(Using Gerrit), 2016-08-03 02:42:52]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-03 02:43:13]

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

[leonhsl(Using Gerrit), 2016-08-03 03:38:48]

Uploaded ps#3 to address comments, PTAnL, Thanks.

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
File content/browser/leveldb_wrapper_impl.cc (right):

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:43: count += (pair.first.size(),
pair.second.value().size());
On 2016/08/02 20:31:54, michaeln wrote:
> On 2016/08/02 16:20:09, yzshen1 wrote:
> > pair.second may be null, right? (e.g., see line 161) It seems you cannot
> > de-reference it directly in that case.
> 
> also, this line looks weird, can you fix this to clearly add both size values
> 
>  count += first.size;
>  if (second)
>    count += second.value().size;

Done. And also confirmed there has no similar problem at other places about
possible null value in changed_values.

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:100: if (*(found->second) == value) {
On 2016/08/02 20:31:54, michaeln wrote:
> On 2016/08/02 16:20:09, yzshen1 wrote:
> > It seems here we also assume that found->second is not null. Is that
intended?
> > 
> > (And also line 164, 210, 226)
> 
> Values in this map_ may not be null, it might be good to DCHECK() that under
> line 99.
> 
> Values in the CommitBatch's map may be null, thats how deletes are
represented.

Yeah I think DCHECK() is proper here, as map_ is supposed always containing no
null value. Done and also handled other similar places. Thanks!

[commit-bot: I haz the power, 2016-08-03 03:56:44]

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

[commit-bot: I haz the power, 2016-08-03 03:56:45]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-08-03 17:23:05]

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
File content/browser/leveldb_wrapper_impl.cc (right):

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:100: if (*(found->second) == value) {
On 2016/08/03 03:38:48, leonhsl wrote:
> On 2016/08/02 20:31:54, michaeln wrote:
> > On 2016/08/02 16:20:09, yzshen1 wrote:
> > > It seems here we also assume that found->second is not null. Is that
> intended?
> > > 
> > > (And also line 164, 210, 226)
> > 
> > Values in this map_ may not be null, it might be good to DCHECK() that under
> > line 99.
> > 
> > Values in the CommitBatch's map may be null, thats how deletes are
> represented.
> 
> Yeah I think DCHECK() is proper here, as map_ is supposed always containing no
> null value. Done and also handled other similar places. Thanks!

If map_ is not supposed to have null value, it would be better to not use
base::Optional<> for its value type. That will be more straightforward. WDYT?

[leonhsl(Using Gerrit), 2016-08-08 08:37:32]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 08:37: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, 2016-08-08 08:39:32]

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

[commit-bot: I haz the power, 2016-08-08 08:39:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[leonhsl(Using Gerrit), 2016-08-08 09:12:03]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

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

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, 2016-08-08 09:21:13]

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

[commit-bot: I haz the power, 2016-08-08 09:21:15]

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

[leonhsl(Using Gerrit), 2016-08-08 09:42:19]

Description was changed from

==========
Switch on use_new_wrapper_types mode for content/common.

Uses STL vector/string types instead of mojo::{Array,String}
for all mojoms in content/common.
  content/common/frame.mojom
  content/common/render_frame_message_filter.mojom
  content/common/render_widget_window_tree_client_factory.mojom
  content/common/service_worker/embedded_worker_setup.mojom
  content/common/storage_partition_service.mojom
  content/common/leveldb_wrapper.mojom
  content/common/image_downloader/image_downloader.mojom
  content/common/process_control.mojom

BUG=624136
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Switch on use_new_wrapper_types mode for content/common.

Uses STL vector/string types instead of mojo::{Array,String}
for all mojoms in content/common.
  content/common/frame.mojom
  content/common/render_frame_message_filter.mojom
  content/common/render_widget_window_tree_client_factory.mojom
  content/common/service_worker/embedded_worker_setup.mojom
  content/common/storage_partition_service.mojom
  content/common/leveldb_wrapper.mojom
  content/common/image_downloader/image_downloader.mojom
  content/common/process_control.mojom
  content/common/websocket.mojom

BUG=624136
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[leonhsl(Using Gerrit), 2016-08-09 02:39:56]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 02:40: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, 2016-08-09 02:44:03]

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

[commit-bot: I haz the power, 2016-08-09 02:44:04]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  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_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[leonhsl(Using Gerrit), 2016-08-09 02:54:05]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 02:54:18]

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, 2016-08-09 03:05:17]

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

[commit-bot: I haz the power, 2016-08-09 03:05:18]

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

[leonhsl(Using Gerrit), 2016-08-09 04:54:33]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 04:54:57]

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, 2016-08-09 05:06:30]

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

[commit-bot: I haz the power, 2016-08-09 05:06:32]

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

[leonhsl(Using Gerrit), 2016-08-09 05:33:23]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 05:33:57]

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, 2016-08-09 06:53:33]

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

[commit-bot: I haz the power, 2016-08-09 06:53:35]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[leonhsl(Using Gerrit), 2016-08-09 07:19:30]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 07:20:07]

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, 2016-08-09 07:50:33]

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

[commit-bot: I haz the power, 2016-08-09 07:50:34]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-08-09 07:54:35]

Description was changed from

==========
Switch on use_new_wrapper_types mode for content/common.

Uses STL vector/string types instead of mojo::{Array,String}
for all mojoms in content/common.
  content/common/frame.mojom
  content/common/render_frame_message_filter.mojom
  content/common/render_widget_window_tree_client_factory.mojom
  content/common/service_worker/embedded_worker_setup.mojom
  content/common/storage_partition_service.mojom
  content/common/leveldb_wrapper.mojom
  content/common/image_downloader/image_downloader.mojom
  content/common/process_control.mojom
  content/common/websocket.mojom

BUG=624136
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Switch on use_new_wrapper_types mode for content/common.

Uses STL vector/string types instead of mojo::{Array,String}
for all mojoms in content/common.
  content/common/frame.mojom
  content/common/render_frame_message_filter.mojom
  content/common/render_widget_window_tree_client_factory.mojom
  content/common/service_worker/embedded_worker_setup.mojom
  content/common/storage_partition_service.mojom
  content/common/leveldb_wrapper.mojom
  content/common/image_downloader/image_downloader.mojom
  content/common/process_control.mojom

BUG=624136
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[leonhsl(Using Gerrit), 2016-08-09 07:56:51]

Uploaded ps#4 to address comments from Yuzhu, and several other ps to do rebase.
Would you PTAnL? Thanks.

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
File content/browser/leveldb_wrapper_impl.cc (right):

https://codereview.chromium.org/2201763002/diff/20001/content/browser/leveldb...
content/browser/leveldb_wrapper_impl.cc:100: if (*(found->second) == value) {
On 2016/08/03 17:23:05, yzshen1 wrote:
> On 2016/08/03 03:38:48, leonhsl wrote:
> > On 2016/08/02 20:31:54, michaeln wrote:
> > > On 2016/08/02 16:20:09, yzshen1 wrote:
> > > > It seems here we also assume that found->second is not null. Is that
> > intended?
> > > > 
> > > > (And also line 164, 210, 226)
> > > 
> > > Values in this map_ may not be null, it might be good to DCHECK() that
under
> > > line 99.
> > > 
> > > Values in the CommitBatch's map may be null, thats how deletes are
> > represented.
> > 
> > Yeah I think DCHECK() is proper here, as map_ is supposed always containing
no
> > null value. Done and also handled other similar places. Thanks!
> 
> If map_ is not supposed to have null value, it would be better to not use
> base::Optional<> for its value type. That will be more straightforward. WDYT?

Sorry for late response. Agree and addressed this at ps#4. Done.

[yzshen1, 2016-08-09 16:14:34]

lgtm

[Lei Zhang, 2016-08-09 16:20:20]

Not sure why I'm on the reviewers list. Is there something in particular I
should look at?

[leonhsl(Using Gerrit), 2016-08-10 02:52:08]

leon.han@intel.com changed reviewers:
- thestig@chromium.org

[leonhsl(Using Gerrit), 2016-08-10 02:52:09]

As this CL has no changes for base/ now, -thestig@ from reviewers list.

And, soft ping John and Michael, Thanks.

[michaeln, 2016-08-10 19:59:04]

lgtm

[leonhsl(Using Gerrit), 2016-08-11 11:38:31]

Ping John for rubber stamp of content/ ;-)

[leonhsl(Using Gerrit), 2016-08-15 02:39:50]

leon.han@intel.com changed reviewers:
+ kinuko@chromium.org
- jam@chromium.org

[leonhsl(Using Gerrit), 2016-08-15 02:39:51]

-jam@, I suppose I'd better invite more specific content/ OWNER instead of John.
+kinuko@, Would you PTAL for OWNER review of content/? Thanks.

[leonhsl(Using Gerrit), 2016-08-15 05:26:08]

Description was changed from

==========
Switch on use_new_wrapper_types mode for content/common.

Uses STL vector/string types instead of mojo::{Array,String}
for all mojoms in content/common.
  content/common/frame.mojom
  content/common/render_frame_message_filter.mojom
  content/common/render_widget_window_tree_client_factory.mojom
  content/common/service_worker/embedded_worker_setup.mojom
  content/common/storage_partition_service.mojom
  content/common/leveldb_wrapper.mojom
  content/common/image_downloader/image_downloader.mojom
  content/common/process_control.mojom

BUG=624136
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Switch on use_new_wrapper_types mode for content/common.

Uses STL vector/string types instead of mojo::{Array,String}
for all mojoms in content/common.
  content/common/frame.mojom
  content/common/render_frame_message_filter.mojom
  content/common/render_widget_window_tree_client_factory.mojom
  content/common/service_worker/embedded_worker_setup.mojom
  content/common/storage_partition_service.mojom
  content/common/leveldb_wrapper.mojom
  content/common/image_downloader/image_downloader.mojom
  content/common/websocket.mojom

BUG=624136
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[leonhsl(Using Gerrit), 2016-08-15 05:31:41]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 05:31:49]

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, 2016-08-15 06:33:06]

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

[commit-bot: I haz the power, 2016-08-15 06:33:07]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[leonhsl(Using Gerrit), 2016-08-15 07:12:01]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 07:12:07]

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, 2016-08-15 08:02:32]

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

[commit-bot: I haz the power, 2016-08-15 08:02:33]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[leonhsl(Using Gerrit), 2016-08-15 08:32:06]

Hi, Yuzhu, would you PTAL at the trybot compile error? Thanks.

Seems compiler is trying to compile the default impl of
WebSocketHandshakeRequest copy constructor/assigner, but as HttpHeaderPtr is
non-copiable, so does std::vector<HttpHeaderPtr>, so caused failure. 
Maybe we need to mark copy constructor/assigner as 'delete' for all generated
struct?

"
class WebSocketHandshakeRequest {
  GURL url;
  std::vector<HttpHeaderPtr> headers;
  std::string headers_text;
};
"
is generated out by bellowing mojom definitions:
"
struct HttpHeader {
  string name;
  string value;
};

struct WebSocketHandshakeRequest {
  url.mojom.Url url;
  array<HttpHeader> headers;
  string headers_text;
};
"

[yzshen1, 2016-08-15 17:23:49]

On 2016/08/15 08:32:06, leonhsl wrote:
> Hi, Yuzhu, would you PTAL at the trybot compile error? Thanks.
> 
> Seems compiler is trying to compile the default impl of
> WebSocketHandshakeRequest copy constructor/assigner, but as HttpHeaderPtr is
> non-copiable, so does std::vector<HttpHeaderPtr>, so caused failure. 
> Maybe we need to mark copy constructor/assigner as 'delete' for all generated
> struct?
> 
> "
> class WebSocketHandshakeRequest {
>   GURL url;
>   std::vector<HttpHeaderPtr> headers;
>   std::string headers_text;
> };
> "
> is generated out by bellowing mojom definitions:
> "
> struct HttpHeader {
>   string name;
>   string value;
> };
> 
> struct WebSocketHandshakeRequest {
>   url.mojom.Url url;
>   array<HttpHeader> headers;
>   string headers_text;
> };
> "

looking now, sorry for inconvenience.

[yzshen1, 2016-08-15 17:23:52]

On 2016/08/15 08:32:06, leonhsl wrote:
> Hi, Yuzhu, would you PTAL at the trybot compile error? Thanks.
> 
> Seems compiler is trying to compile the default impl of
> WebSocketHandshakeRequest copy constructor/assigner, but as HttpHeaderPtr is
> non-copiable, so does std::vector<HttpHeaderPtr>, so caused failure. 
> Maybe we need to mark copy constructor/assigner as 'delete' for all generated
> struct?
> 
> "
> class WebSocketHandshakeRequest {
>   GURL url;
>   std::vector<HttpHeaderPtr> headers;
>   std::string headers_text;
> };
> "
> is generated out by bellowing mojom definitions:
> "
> struct HttpHeader {
>   string name;
>   string value;
> };
> 
> struct WebSocketHandshakeRequest {
>   url.mojom.Url url;
>   array<HttpHeader> headers;
>   string headers_text;
> };
> "

looking now, sorry for inconvenience.

[yzshen1, 2016-08-15 20:02:29]

Hi,

I have got a fix pending for review: https://codereview.chromium.org/2244363003/

[kinuko, 2016-08-15 23:05:55]

lgtm % some nits

https://codereview.chromium.org/2201763002/diff/160001/content/renderer/dom_s...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/2201763002/diff/160001/content/renderer/dom_s...
content/renderer/dom_storage/local_storage_cached_area.cc:28: return
base::string16(reinterpret_cast<const base::char16*>(&input.front()),
nit: use input.data() instead of &input.front() and remove if (input.empty())
condition?

https://codereview.chromium.org/2201763002/diff/160001/content/renderer/dom_s...
content/renderer/dom_storage/local_storage_cached_area.cc:35:
memcpy(&result.front(), input.c_str(), input.size() * sizeof(base::char16));
Hmm... could something like this work?

const uint8_t* data = reinterpret_cast<const uint8_t*>(input.data());
return std::vector<uint8_t>(data, data + input.size() * sizeof(base::char16));

[leonhsl(Using Gerrit), 2016-08-16 02:05:44]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 02:06:07]

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

[leonhsl(Using Gerrit), 2016-08-16 02:39:28]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 02:39: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, 2016-08-16 03:48:44]

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

[commit-bot: I haz the power, 2016-08-16 03:48:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[leonhsl(Using Gerrit), 2016-08-16 03:51:04]

The CQ bit was checked by leon.han@intel.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 03:51:22]

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, 2016-08-16 04:25:47]

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

[commit-bot: I haz the power, 2016-08-16 04:25:48]

Dry run: This issue passed the CQ dry run.

[leonhsl(Using Gerrit), 2016-08-16 05:54:33]

Thanks all! Uploaded ps#10 to address nits from kinuko@ and all trybots got
green, will land now.

https://codereview.chromium.org/2201763002/diff/160001/content/renderer/dom_s...
File content/renderer/dom_storage/local_storage_cached_area.cc (right):

https://codereview.chromium.org/2201763002/diff/160001/content/renderer/dom_s...
content/renderer/dom_storage/local_storage_cached_area.cc:28: return
base::string16(reinterpret_cast<const base::char16*>(&input.front()),
On 2016/08/15 23:05:55, kinuko wrote:
> nit: use input.data() instead of &input.front() and remove if (input.empty())
> condition?

Done.

https://codereview.chromium.org/2201763002/diff/160001/content/renderer/dom_s...
content/renderer/dom_storage/local_storage_cached_area.cc:35:
memcpy(&result.front(), input.c_str(), input.size() * sizeof(base::char16));
On 2016/08/15 23:05:55, kinuko wrote:
> Hmm... could something like this work?
> 
> const uint8_t* data = reinterpret_cast<const uint8_t*>(input.data());
> return std::vector<uint8_t>(data, data + input.size() * sizeof(base::char16));

Done and thanks! The code looks cleaner.

[leonhsl(Using Gerrit), 2016-08-16 05:55:27]

The CQ bit was checked by leon.han@intel.com

[leonhsl(Using Gerrit), 2016-08-16 05:55:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, yzshen@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2201763002/#ps180001
(title: "Address comments from kinuko")

[commit-bot: I haz the power, 2016-08-16 05:55:42]

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-08-16 05:59:40]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-08-16 06:02:10]

Description was changed from

==========
Switch on use_new_wrapper_types mode for content/common.

Uses STL vector/string types instead of mojo::{Array,String}
for all mojoms in content/common.
  content/common/frame.mojom
  content/common/render_frame_message_filter.mojom
  content/common/render_widget_window_tree_client_factory.mojom
  content/common/service_worker/embedded_worker_setup.mojom
  content/common/storage_partition_service.mojom
  content/common/leveldb_wrapper.mojom
  content/common/image_downloader/image_downloader.mojom
  content/common/websocket.mojom

BUG=624136
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Switch on use_new_wrapper_types mode for content/common.

Uses STL vector/string types instead of mojo::{Array,String}
for all mojoms in content/common.
  content/common/frame.mojom
  content/common/render_frame_message_filter.mojom
  content/common/render_widget_window_tree_client_factory.mojom
  content/common/service_worker/embedded_worker_setup.mojom
  content/common/storage_partition_service.mojom
  content/common/leveldb_wrapper.mojom
  content/common/image_downloader/image_downloader.mojom
  content/common/websocket.mojom

BUG=624136
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[commit-bot: I haz the power, 2016-08-16 06:02:12]

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