Merge weburlrequest_extradata_impl.cc/h into content/child/request_extra_data.cc/h (attempt #2)

Merge weburlrequest_extradata_impl.cc/h into content/child/request_extra_data.cc/h (attempt #2)

patch from issue 182383015, with changes to plugin_url_fetcher.cc, resource_dispatcher_unittest.cc, and render_frame_impl.cc to create RequestExtraData object on stack instead of with new

BUG=338338
TBR=darin@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259340

[pilgrim_google, 2014-03-18 18:47:37]

Not sure which ASAN bot the original patch failed on. Can someone add the
correct trybot?

[pilgrim_google, 2014-03-18 18:56:11]

Also, this doesn't compile because the empty constructor is interpreted as a
function declaration. :(

[yzshen1, 2014-03-18 19:00:55]

On 2014/03/18 18:47:37, pilgrim_google wrote:
> Not sure which ASAN bot the original patch failed on. Can someone add the
> correct trybot?

http://build.chromium.org/p/chromium.memory/buildstatus?builder=Linux%20ASan%...

I sent a mail with this link to the revert CL, but the link doesn't work.

Let's see whether it works by directly replying on the web page....

[yzshen1, 2014-03-18 19:03:17]

On 2014/03/18 19:00:55, yzshen1 wrote:
> On 2014/03/18 18:47:37, pilgrim_google wrote:
> > Not sure which ASAN bot the original patch failed on. Can someone add the
> > correct trybot?
> 
>
http://build.chromium.org/p/chromium.memory/buildstatus?builder=Linux%2520ASa...
> 
> I sent a mail with this link to the revert CL, but the link doesn't work.
> 
> Let's see whether it works by directly replying on the web page....

Nope, still doesn't work on the web page.
But it should work in your email notification.

I should file a bug with the codereview tool if there isn't one already.

[jam, 2014-03-18 21:26:41]

hi, see Michael's and my post in the original change. with those changes, lgtm

[pilgrim_google, 2014-03-19 19:56:52]

The CQ bit was checked by pilgrim@chromium.org

[commit-bot: I haz the power, 2014-03-19 19:58:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/203853003/30001

[commit-bot: I haz the power, 2014-03-19 20:01:48]

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

[commit-bot: I haz the power, 2014-03-19 20:01:49]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[pilgrim_google, 2014-03-19 20:30:46]

The CQ bit was checked by pilgrim@chromium.org

[commit-bot: I haz the power, 2014-03-19 20:31:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/203853003/30001

[commit-bot: I haz the power, 2014-03-19 20:37:34]

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

[commit-bot: I haz the power, 2014-03-19 20:37:34]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[michaeln, 2014-03-20 00:10:42]

https://codereview.chromium.org/203853003/diff/30001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/203853003/diff/30001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1929: RequestExtraData extra_data;
whoa... this is stack allocated shouldn't this be heap alloced? what am i
missing?

    // Extra data associated with the underlying resource request. Resource
    // requests can be copied. If non-null, each copy of a resource requests
    // holds a pointer to the extra data, and the extra data pointer will be
    // deleted when the last resource request is destroyed. Setting the extra
    // data pointer will cause the underlying resource request to be
    // dissociated from any existing non-null extra data pointer.
    BLINK_PLATFORM_EXPORT ExtraData* extraData() const;
    BLINK_PLATFORM_EXPORT void setExtraData(ExtraData*);

[michaeln, 2014-03-20 00:48:25]

https://codereview.chromium.org/203853003/diff/30001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/203853003/diff/30001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1929: RequestExtraData extra_data;
On 2014/03/20 00:10:43, michaeln wrote:
> whoa... this is stack allocated shouldn't this be heap alloced? what am i
> missing?
> 
>     // Extra data associated with the underlying resource request. Resource
>     // requests can be copied. If non-null, each copy of a resource requests
>     // holds a pointer to the extra data, and the extra data pointer will be
>     // deleted when the last resource request is destroyed. Setting the extra
>     // data pointer will cause the underlying resource request to be
>     // dissociated from any existing non-null extra data pointer.
>     BLINK_PLATFORM_EXPORT ExtraData* extraData() const;
>     BLINK_PLATFORM_EXPORT void setExtraData(ExtraData*);

Let me rephrase that, its not really a question.

Please heap allocate the extra_data here since ownership of the ptr is passed to
the blink::WebURLRequest below. And keep as stack allocs in
plugin_url_fetcher.cc and resource_dispatcher_unittest.cc (which do not pass
ownership to WebURLRequest).

[pilgrim_google, 2014-03-20 14:54:19]

On 2014/03/20 00:48:25, michaeln wrote:
> Please heap allocate the extra_data here since ownership of the ptr is passed
to
> the blink::WebURLRequest below. And keep as stack allocs in
> plugin_url_fetcher.cc and resource_dispatcher_unittest.cc (which do not pass
> ownership to WebURLRequest).

Done. Please re-review.

[jam, 2014-03-20 17:16:43]

https://codereview.chromium.org/203853003/diff/50001/content/child/request_ex...
File content/child/request_extra_data.cc (right):

https://codereview.chromium.org/203853003/diff/50001/content/child/request_ex...
content/child/request_extra_data.cc:15:
set_visibility_state(blink::WebPageVisibilityStateVisible);
use initialization list here, i.e.
RequestExtraData::RequestExtraData()
   : visibility_state_(blink::WebPageVisibilityStateVisible),
     render_frame_id_(MSG_ROUTING_NONE),

etc

no need to initialize classes with empty constructors since that happens
automatically (i.e. the GURL, WebString types). also make sure you follow the
same order as the header

https://codereview.chromium.org/203853003/diff/50001/content/child/request_ex...
content/child/request_extra_data.cc:31: void
RequestExtraData::set_visibility_state(
since these are simple setters, by convention we inline them in the header

[michaeln, 2014-03-24 19:53:45]

any chance this can get committed soon?

[pilgrim_google, 2014-03-24 20:56:32]

On 2014/03/24 19:53:45, michaeln wrote:
> any chance this can get committed soon?

Yup.

[pilgrim_google, 2014-03-25 16:28:37]

The CQ bit was checked by pilgrim@chromium.org

[commit-bot: I haz the power, 2014-03-25 16:30:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/203853003/70001

[jam, 2014-03-25 17:30:06]

https://codereview.chromium.org/203853003/diff/70001/content/child/request_ex...
File content/child/request_extra_data.h (right):

https://codereview.chromium.org/203853003/diff/70001/content/child/request_ex...
content/child/request_extra_data.h:52: return custom_user_agent_;
nit: here and in the next few methods, indent 2 spaces only not 4

https://codereview.chromium.org/203853003/diff/70001/content/child/request_ex...
content/child/request_extra_data.h:59: void set_render_frame_id(int
render_frame_id) {
note: the convention is that all the setters/getters are listed in the same
order as the member variables. also, each setter and getter for a variable
should be right beside each other, i.e.

 public:
  int x() { return x_;}
  void set_x(int x) { x_ = x; }
  int y() { return y_;}
  void set_y(int y) { y_ = y; }
  int z() { return z_;}
  void set_z(int z) { z_ = z; }

 private:
  int x_;
  int y_;
  int z_;

https://codereview.chromium.org/203853003/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/url_request_info_util.cc (right):

https://codereview.chromium.org/203853003/diff/70001/content/renderer/pepper/...
content/renderer/pepper/url_request_info_util.cc:179: RequestExtraData*
extraData = new RequestExtraData();
nit: extra_data per style guide

[jam, 2014-03-25 17:30:13]

The CQ bit was unchecked by jam@chromium.org

[pilgrim_google, 2014-03-25 18:49:31]

The CQ bit was checked by pilgrim@chromium.org

[commit-bot: I haz the power, 2014-03-25 18:50:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pilgrim@chromium.org/203853003/260001

[jam, 2014-03-25 19:24:03]

lgtm

[commit-bot: I haz the power, 2014-03-25 21:35:05]

Change committed as 259340