Split BrowserThread into public API and private implementation, step 1.

[Jói Sigurðsson]

Hi John,

A fun review for you today :)

The change is way too big for Rietveld, so attaching raw patch. I
recommend a highlighting patch file viewer such as Emacs, that makes
it a little better.

The interesting bits are in these files:
content/public/browser/browser_thread.h
content/browser/browser_thread_impl.h
content/browser/browser_thread_impl.cc
content/test/test_browser_thread.h
content/test/test_browser_thread.cc

The bulk of the rest is switching tests to use
content::TestBrowserThread, and chrome/ production code to use
DeprecatedBrowserThread, this latter thing being temporary.

I haven't switched BrowserThread to the content namespace. If we want
this (I guess we do?), I will do it as a follow-up.

Cheers,
Jói

=== change description ===
Split BrowserThread into public API and private implementation, step 1.

Only content/ now has the ability to create BrowserThread objects,
with the exception that tests can create the
content::TestBrowserThread subclass, and (temporarily) code in chrome/
can create the DeprecatedBrowserThread subclass.

A follow-up change will make content/ take care of its own thread
creation, remove DeprecatedBrowserThread, and move all state and
non-trivial constructors from BrowserThread down to BrowserThreadImpl.

Also moved BrowserProcessSubThread into content/ namespace. As part
of follow-up cleanup, chrome/ will stop using this class.

BUG=98716
TEST=existing

[Jói Sigurðsson]

FYI, I ran sort-headers.py on all changed files. My update to the
tool to help do this was accidentally included in the patch, I've
removed it from the branch I'm working on.

Cheers,
Jói

2011/10/26 Jói Sigurðsson <j...@google.com>:

[Jói Sigurðsson]

Now from, and to, right addresses.

2011/10/26 Jói Sigurðsson <j...@google.com>:

[John Abd-El-Malek]

(please start reading the inline comments in your message first! i've only looked at the 6 files you've mentioned)

One of the conventions that I've been trying to copy from WebKit API is to not have content code use the content API directly. This makes it easy to change content code in the future to access implementation methods without having to update includes or end up with some code in one file calling the interface while another part of the file calls the implementation. also it just seems cumbersome to make other implementation code in content go through the API. Sorry this stuff isn't on a wiki yet, I'm keeping it in a text file for when we accumulate a bunch of guidelines, but perhaps I should keep it in the wiki from now.

+// A BrowserThread for unit tests; this lets unit tests in chrome/

+// create BrowserThread instances.

+class TestBrowserThread : public BrowserThreadImpl {

Using inheritance has the side-effect of allowing non-test code to include browser_thread.h indirectly. I've tried to avoid chrome getting to content implementation headers in other test classes. We should be able to use composition here, right? I've even tried to not leak the content implementation names into chrome, i.e. in cases like this I make the test object hold a pointer to the base class instead of forwarding declaring the Impl.

+// Temporary escape hatch for chrome/ to construct BrowserThread,

+// until we make content/ construct its own threads.

+class DeprecatedBrowserThread : public BrowserThread {

I'm curious: why add this instead of just having chrome create BrowserThreadImpl directly in the meantime? It seems like a bit of extra work, with not much benefit, to add temporary classes.

+class CONTENT_EXPORT BrowserThread : public base::Thread {

Why does the interface derive from base::Thread and have member variables and non-static function instead of having all of these be in BrowserThreadImpl?

On Wed, Oct 26, 2011 at 4:17 PM, Jói Sigurðsson <j...@chromium.org> wrote:

Now from, and to, right addresses.

2011/10/26 Jói Sigurðsson <j...@google.com>:

> Hi John,
>
> A fun review for you today :)
>
> The change is way too big for Rietveld, so attaching raw patch.  I
> recommend a highlighting patch file viewer such as Emacs, that makes
> it a little better.
>
> The interesting bits are in these files:
> content/public/browser/browser_thread.h
> content/browser/browser_thread_impl.h
> content/browser/browser_thread_impl.cc
> content/test/test_browser_thread.h
> content/test/test_browser_thread.cc
>
> The bulk of the rest is switching tests to use
> content::TestBrowserThread, and chrome/ production code to use
> DeprecatedBrowserThread, this latter thing being temporary.
>
> I haven't switched BrowserThread to the content namespace.  If we want
> this (I guess we do?),

yes everything in the public directory is in the content namespace

 

I will do it as a follow-up.

doing this in one huge swoop is very difficult, especially for BrowserThread which is used by so many files. I highly suggest splitting adding the interface from updating all the code for this reason. you can make one first patch which creates the interface and puts in the content namespace. in the top of the file you can put "using content::BrowserThread". keep the old file in place and just have it include the new one. then in future patches, which you can TBR, update the users. I had to do this when moving browserthread initially from chrome to content.

 

>
> Cheers,
> Jói
>
>
> === change description ===
> Split BrowserThread into public API and private implementation, step 1.
>

It's much easier for me as a reviewer, and you as a commiter if changes that touch so many files do this in smaller steps. i.e.

 

> Only content/ now has the ability to create BrowserThread objects,

step 1

 

> with the exception that tests can create the
> content::TestBrowserThread subclass,

step 2

 

and (temporarily) code in chrome/
> can create the

 

subclass.

step 3

 

If it's not too much effort, would it be possible to do this for this change? Then Rietveld might be able to cope with the number of files, which would make my life much easier :) Once the new classes are added, then switching existing code can be done in TBR patches so as to not slow you down, and also makes it easier for you since they're smaller. By the way, with large files like this, I usually upload with "--no-presubmit" and also remove the entries in WATCHLIST. I've been able to upload ~600 file patches this way.

[John Abd-El-Malek]

On Wed, Oct 26, 2011 at 7:04 PM, John Abd-El-Malek <j...@chromium.org> wrote:

(please start reading the inline comments in your message first! i've only looked at the 6 files you've mentioned)

One of the conventions that I've been trying to copy from WebKit API is to not have content code use the content API directly. This makes it easy to change content code in the future to access implementation methods without having to update includes or end up with some code in one file calling the interface while another part of the file calls the implementation. also it just seems cumbersome to make other implementation code in content go through the API. Sorry this stuff isn't on a wiki yet, I'm keeping it in a text file for when we accumulate a bunch of guidelines, but perhaps I should keep it in the wiki from now.

I've updated the Content API wiki with the conventions in the API: https://sites.google.com/a/chromium.org/dev/developers/content-module/content-api?pli=1

[Jói Sigurðsson]

>> One of the conventions that I've been trying to copy from WebKit API is to

>> not have content code use the content API directly. [...]

That makes sense, and it's not a huge deal to change it. I'll switch
to that approach as a follow-up change, in the interest of landing
this one as early as possible today, before MTV and the tree get busy.

>> +class TestBrowserThread : public BrowserThreadImpl {
>> Using inheritance has the side-effect of allowing non-test code to include
>> browser_thread.h indirectly. I've tried to avoid chrome getting to content

>> implementation headers in other test classes. [...]

Since this is under content/test/, this shouldn't be an issue once we
update the deps tool to know that only
_(api|browser|unit|etc)test.(h|cc) files should be depending on
content/test. However, I'm happy to switch to using composition if
that is greatly preferred, but would like to do so in a follow-up
change.

>> +class DeprecatedBrowserThread : public BrowserThread {
>> I'm curious: why add this instead of just having chrome create
>> BrowserThreadImpl directly in the meantime? It seems like a bit of extra
>> work, with not much benefit, to add temporary classes.

It's almost zero extra work, and it lets me finish updating all the
include paths, plus it means I can effortlessly find all the places I
already identified as "production" (rather than test) users in chrome/
of BrowserThread. This is such a big change that it needs to be
iterated on anyway; I will get rid of DeprecatedBrowserThread soon.

>> +class CONTENT_EXPORT BrowserThread : public base::Thread {
>> Why does the interface derive from base::Thread and have member variables
>> and non-static function instead of having all of these be in
>> BrowserThreadImpl?

I believe in at least a few places in chrome/, the Thread methods are
being used on BrowserThread (now DeprecatedBrowserThread) objects.
This is one of the things I can move down to BrowserThreadImpl when I
get rid of chrome/'s dependency on DeprecatedBrowserThread.

>>> > I haven't switched BrowserThread to the content namespace.  If we want
>>> > this (I guess we do?),
>>
>> yes everything in the public directory is in the content namespace

Cool, will do that as a follow-up.

>> [...] keep the old file in place and just have it include

>> the new one. then in future patches, which you can TBR, update the users. I
>> had to do this when moving browserthread initially from chrome to content.

Right, in retrospect this would have been better, but now that I have
the change ready to land it would be something close to a day's worth
of work to re-do it that way. I guess you learned the lesson already
the hard way, and now I'm learning it the same way, but from the
present point it seems like a lot less work to land the change more or
less as is and then iterate on it, rather than break it into pieces.

>>> > Only content/ now has the ability to create BrowserThread objects,
>>
>> step 1
>>
>>>
>>> > with the exception that tests can create the
>>> > content::TestBrowserThread subclass,
>>
>> step 2
>>
>>>
>>> and (temporarily) code in chrome/
>>> > can create the
>>
>>
>>>
>>> subclass.
>>
>> step 3
>>
>> If it's not too much effort, would it be possible to do this for this
>> change? Then Rietveld might be able to cope with the number of files, which
>> would make my life much easier :) Once the new classes are added, then
>> switching existing code can be done in TBR patches so as to not slow you
>> down, and also makes it easier for you since they're smaller. By the way,
>> with large files like this, I usually upload with "--no-presubmit" and also
>> remove the entries in WATCHLIST. I've been able to upload ~600 file patches
>> this way.

I estimate it would be 4-6 good hours of re-work to break it up, so I
think that's too much. In any case, step 1 requires steps 2 and 3 for
things to compile. I guess I could have done it in the reverse order
(allowing BrowserThread construction while doing steps 2 and 3, then
doing step 1) but it's kind of too late for that now - but something I
will certainly learn from for next time.

BTW, I did try a few times to upload to Rietveld with --bypass-hooks
and with --cc set to a short list (this overrides the WATCHLISTS I
think, otherwise you get a CC line longer than 1000 characters which
Rietveld won't accept). It was erroring out with a 500 after a few
dozen files.

I will spend a little time before you get in to make the review as
quick and painless as possible, both to save you time and so that the
change can go in early this morning MTV time. Will update here once I
have some kind of solution for that.

Cheers,
Jói

2011/10/27 John Abd-El-Malek <j...@chromium.org>:

[Jói Sigurðsson]

I extracted a separate patch that contains the salient bits of this
one and uploaded to Rietveld, see
http://codereview.chromium.org/8340028/. I guess we should keep most
comments on this, the original review thread.

Apart from that Rietveld review, I also posted this to the other
review thread, but meant to post it here, so I'm repeating myself to
make sure most info stays on the main review thread:

=============
Here's another way to look at the change more quickly:

I wrote the attached script (review_helper.py), thinking it might be
nice to customize for more of our move-style changes. You can verify
the rules it uses to ignore files pretty easily: If all + lines
contain one of the patterns mentioned in CategorizePerFileDiffs (and
there are some extra patterns just for files containing "test.") then
that file is considered a "search and replace" file and goes to the
.searchandreplace patch. Otherwise the diff for that whole file goes
to the .manual part of the patch. This way you only need to look at a
96K patch file, and can just skim the other 501K file.

Attached is the script, the original patch (most current version) and
the .manual and .searchandreplace parts.
=============

Cheers,
Jói

2011/10/27 Jói Sigurðsson <j...@chromium.org>:

[John Abd-El-Malek]

lgtm to get this landed while these tasks are done in a separate patch per our discussion

in general i would have really preferred if this went in in more steps, but now that you've done it this way, i agree with you that doing it differently now would be a time sink. if you can at least make content tests not use the test browser thread class that would be good, but not necessary.

i know i said earlier that one of our goals is to make content impl not use the APIs. I'm wondering now if we should make an exception for BrowserThread when using it to post tasks since these are all static methods? it might make more consistency in code between content and chrome. wdyt?

On Thu, Oct 27, 2011 at 6:56 AM, Jói Sigurðsson <j...@chromium.org> wrote:

>> One of the conventions that I've been trying to copy from WebKit API is to

>> not have content code use the content API directly. [...]

That makes sense, and it's not a huge deal to change it.  I'll switch
to that approach as a follow-up change, in the interest of landing
this one as early as possible today, before MTV and the tree get busy.

>> +class TestBrowserThread : public BrowserThreadImpl {
>> Using inheritance has the side-effect of allowing non-test code to include
>> browser_thread.h indirectly. I've tried to avoid chrome getting to content

>> implementation headers in other test classes. [...]

Since this is under content/test/, this shouldn't be an issue once we
update the deps tool to know that only
_(api|browser|unit|etc)test.(h|cc) files should be depending on
content/test.  However, I'm happy to switch to using composition if
that is greatly preferred, but would like to do so in a follow-up
change.

yeah i think we also dont want chrome tests to reach into content implementations. i.e. we should completely separate the content internals from chrome code, whether it's shipping code or tests.

[Jói Sigurðsson]

Thanks for the monster review John!

> in general i would have really preferred if this went in in more steps, but
> now that you've done it this way, i agree with you that doing it differently

> now would be a time sink. [...]

Agreed; lesson learned.

> i know i said earlier that one of our goals is to make content impl not use
> the APIs. I'm wondering now if we should make an exception for BrowserThread
> when using it to post tasks since these are all static methods? it might
> make more consistency in code between content and chrome. wdyt?

I think so.

> [...] if you can at least make content tests not use the

> test browser thread class that would be good, but not necessary.

I'll make this change before committing, unless it turns out to be so
non-trivial as to need a new review, in which case I'll do it as a
follow-up.

> yeah i think we also dont want chrome tests to reach into content

> implementations. i.e. we should completely separate the content internals
> from chrome code, whether it's shipping code or tests.

Will look into this as a follow-up. I think I saw stuff that would
make this non-trivial.

[John Abd-El-Malek]

On Thu, Oct 27, 2011 at 10:05 AM, Jói Sigurðsson <j...@chromium.org> wrote:

Thanks for the monster review John!

> in general i would have really preferred if this went in in more steps, but
> now that you've done it this way, i agree with you that doing it differently

> now would be a time sink. [...]

Agreed; lesson learned.

> i know i said earlier that one of our goals is to make content impl not use
> the APIs. I'm wondering now if we should make an exception for BrowserThread
> when using it to post tasks since these are all static methods? it might
> make more consistency in code between content and chrome. wdyt?

I think so.

cool, let's make an exception just for BrowserThread then.

 

> [...] if you can at least make content tests not use the

> test browser thread class that would be good, but not necessary.

I'll make this change before committing, unless it turns out to be so
non-trivial as to need a new review, in which case I'll do it as a
follow-up.

> yeah i think we also dont want chrome tests to reach into content
> implementations. i.e. we should completely separate the content internals
> from chrome code, whether it's shipping code or tests.

Will look into this as a follow-up.  I think I saw stuff that would
make this non-trivial.

yeah this sometimes is a bit of work, i.e. http://codereview.chromium.org/8395038/, but i think this is one rule that we should observe strictly

[Jói Sigurðsson]

Committed as r107718.

I started changing the tests under content/ to use BrowserThreadImpl
but decided to do as a follow-up as I wanted your opinion on something
(using vs. content:: inline in the code), and also because I had green
trybots and a quiet tree. I'll be uploading that change for your
review in just a few minutes.