Revert 97966 : Revert 97955 - First pass on intents options UI.

Revert 97966 : Revert 97955 - First pass on intents options UI.

Original review URL: http://codereview.chromium.org/7624012

R=groby@chromium.org,vandebo@chromium.org
BUG=none
TEST=*Intents*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98340

[Greg Billock, 2011-08-24 23:23:11]

On 2011/08/24 16:04:41, Greg Billock wrote:

OK, there was a JS error that led to these test failures, I think. See the
patchset for it. Now all trybots succeed. (Not sure why they aren't showing up
here...)

[groby-ooo-7-16, 2011-08-25 17:16:29]

On 2011/08/24 23:23:11, Greg Billock wrote:
> On 2011/08/24 16:04:41, Greg Billock wrote:
> 
> OK, there was a JS error that led to these test failures, I think. See the
> patchset for it. Now all trybots succeed. (Not sure why they aren't showing up
> here...)


Fix in content_settings.js seems OK to me.

[vandebo (ex-Chrome), 2011-08-25 17:44:59]

Changes LGTM, feel free to ignore irrelevant nits.

http://codereview.chromium.org/7719029/diff/5001/chrome/browser/ui/intents/in...
File chrome/browser/ui/intents/intents_model.h (right):

http://codereview.chromium.org/7719029/diff/5001/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:22: IntentsTreeNode()
nit: Why not put the implementation in the .cc file?

http://codereview.chromium.org/7719029/diff/5001/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:92: virtual void
TreeModelBeginBatch(IntentsModel* model) {}
nit: Do you need these empty implementations?

http://codereview.chromium.org/7719029/diff/5001/chrome/browser/ui/intents/in...
File chrome/browser/ui/intents/intents_model_unittest.cc (right):

http://codereview.chromium.org/7719029/diff/5001/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model_unittest.cc:45: provider.service_url =
GURL("http://www.google.com/share");
nit: looks like a method to populate a test provider would be useful.  You could
then easily not use extra scopes.

[Greg Billock, 2011-08-25 18:07:43]

http://codereview.chromium.org/7719029/diff/5001/chrome/browser/ui/intents/in...
File chrome/browser/ui/intents/intents_model.h (right):

http://codereview.chromium.org/7719029/diff/5001/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:22: IntentsTreeNode()
On 2011/08/25 17:44:59, vandebo wrote:
> nit: Why not put the implementation in the .cc file?

These are actually in the public api. (You probably didn't read into some of the
usage.) This mirrors the cookies tree API, which is, ahem, closely related. :-)

http://codereview.chromium.org/7719029/diff/5001/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:92: virtual void
TreeModelBeginBatch(IntentsModel* model) {}
On 2011/08/25 17:44:59, vandebo wrote:
> nit: Do you need these empty implementations?

Yes, the TreeModelObserver is a pure virtual interface.

http://codereview.chromium.org/7719029/diff/5001/chrome/browser/ui/intents/in...
File chrome/browser/ui/intents/intents_model_unittest.cc (right):

http://codereview.chromium.org/7719029/diff/5001/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model_unittest.cc:45: provider.service_url =
GURL("http://www.google.com/share");
On 2011/08/25 17:44:59, vandebo wrote:
> nit: looks like a method to populate a test provider would be useful.  You
could
> then easily not use extra scopes.

Good point. That probably belongs in another class, though. I'll go ahead and
keep this for now.

[commit-bot: I haz the power, 2011-08-25 23:02:14]

Change committed as 98340