First pass on intents options UI.

First pass on intents options UI.

Copies the cookies view handler for intents.
Add pass-throughs to grit for flag.
Optionalize intents options page JS/CSS.
Fix up data model to read title. Working test.

R=jhawkins@chromium.org
BUG=None
TEST=*Intents*


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

[James Hawkins, 2011-08-17 03:18:39]

http://codereview.chromium.org/7624012/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7624012/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:5479: Intents
Web Intents

http://codereview.chromium.org/7624012/diff/1/chrome/browser/intents/web_inte...
File chrome/browser/intents/web_intent_data.cc (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/intents/web_inte...
chrome/browser/intents/web_intent_data.cc:15: title == other.title);
There will be a merge conflict with groby's CL.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/content_settings.html (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/content_settings.html:216: <h3
i18n-content="intents_tab_label"></h3>
intentsTabLabel

Here and elsewhere.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/content_settings.js (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/content_settings.js:57:
this.pageDiv.querySelector('#manage-intents-button');
Just use $('manage-intents-button');

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/intents_list.js (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_list.js:19: function
getItemHeight(item) {
Does this need to be refactored into list.js?

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_list.js:33: 
Remove extra blank line.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_list.js:36: * Creates a new list item
for sites data. Note that these are created and
sites?

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_list.js:53: // This hooks up
updateOrigin() to the list item, makes the top-level
Is this file a copy/paste/rename of the cookies code?  If so, we need to do a
refactoring, even for the prototype.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/intents_view.css (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_view.css:20: /* notice the width and
padding for these columns match up with those below */
Capitalize sentence, add period.

HAE

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_view.css:34: border: 1px solid #D9D9D9;
Lower-case letters in hex.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_view.css:52: right: 2px;
RTL

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_view.css:109: display: table-row;
Alphabetize

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/intents_view.html (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_view.html:5: <div
id="intents-data-column"><h3 i18n-content="intents_service_data"></h3></div>
80 cols

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/intents_view.js (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_view.js:6: 
Remove blank line.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options.html (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options.html:164: <include
src="intents_view.html">
Wrap in the pp_ifdef?

http://codereview.chromium.org/7624012/diff/1/chrome/browser/ui/intents/inten...
File chrome/browser/ui/intents/intents_model.cc (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/ui/intents/inten...
chrome/browser/ui/intents/intents_model.cc:125: if (intents.size() == 0) {
Remove this block now that we have the registry.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/ui/intents/inten...
File chrome/browser/ui/intents/intents_model.h (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/ui/intents/inten...
chrome/browser/ui/intents/intents_model.h:45: NodeType type_;
All classes, member vars, methods need documentation.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/ui/intents/inten...
File chrome/browser/ui/intents/intents_model_unittest.cc (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/ui/intents/inten...
chrome/browser/ui/intents/intents_model_unittest.cc:17: 
Deadcode.

http://codereview.chromium.org/7624012/diff/1/content/renderer/render_view.h
File content/renderer/render_view.h (right):

http://codereview.chromium.org/7624012/diff/1/content/renderer/render_view.h#...
content/renderer/render_view.h:443: const WebKit::WebString& type,
This is from the other CL, right?

[Greg Billock, 2011-08-17 18:49:50]

http://codereview.chromium.org/7624012/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7624012/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:5479: Intents
On 2011/08/17 03:18:39, James Hawkins wrote:
> Web Intents

Done.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/intents/web_inte...
File chrome/browser/intents/web_intent_data.cc (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/intents/web_inte...
chrome/browser/intents/web_intent_data.cc:15: title == other.title);
On 2011/08/17 03:18:39, James Hawkins wrote:
> There will be a merge conflict with groby's CL.

If she already fixed the title bug, I'll merge it.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/content_settings.html (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/content_settings.html:216: <h3
i18n-content="intents_tab_label"></h3>
On 2011/08/17 03:18:39, James Hawkins wrote:
> intentsTabLabel
> 
> Here and elsewhere.

Done. Also below.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/content_settings.js (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/content_settings.js:57:
this.pageDiv.querySelector('#manage-intents-button');
On 2011/08/17 03:18:39, James Hawkins wrote:
> Just use $('manage-intents-button');

Done.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/intents_list.js (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_list.js:19: function
getItemHeight(item) {
On 2011/08/17 03:18:39, James Hawkins wrote:
> Does this need to be refactored into list.js?

Seems like a good candidate. Looks like List already has getItemHeight_() (and
that it is more accurate, but perhaps needs to be rendered?). OTOH, only
cookies-list and now this copy use getItemHeight...

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_list.js:33: 
On 2011/08/17 03:18:39, James Hawkins wrote:
> Remove extra blank line.

Done.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_list.js:36: * Creates a new list item
for sites data. Note that these are created and
On 2011/08/17 03:18:39, James Hawkins wrote:
> sites?

Beats me. :-) Fixed this up. BTW, it looks like the move detection isn't
comparing this to cookies_list.js. :-( Perhaps it is too different now.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_list.js:53: // This hooks up
updateOrigin() to the list item, makes the top-level
On 2011/08/17 03:18:39, James Hawkins wrote:
> Is this file a copy/paste/rename of the cookies code?  If so, we need to do a
> refactoring, even for the prototype.

Yes, it's a copy-paste. I don't think I've completely pruned all the stuff we
don't need, though. I suspect it's going to end up quite different, but
commonalities still might be refactorable, perhaps to List as above.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/intents_view.css (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_view.css:20: /* notice the width and
padding for these columns match up with those below */
On 2011/08/17 03:18:39, James Hawkins wrote:
> Capitalize sentence, add period.
> 
> HAE

Done.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_view.css:34: border: 1px solid #D9D9D9;
On 2011/08/17 03:18:39, James Hawkins wrote:
> Lower-case letters in hex.

Done.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_view.css:52: right: 2px;
On 2011/08/17 03:18:39, James Hawkins wrote:
> RTL

The rtl is right below. I assume that's because we can't just say 'end: 2px;'
right?

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_view.css:109: display: table-row;
On 2011/08/17 03:18:39, James Hawkins wrote:
> Alphabetize

Done.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/intents_view.html (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_view.html:5: <div
id="intents-data-column"><h3 i18n-content="intents_service_data"></h3></div>
On 2011/08/17 03:18:39, James Hawkins wrote:
> 80 cols

Done.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/intents_view.js (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/intents_view.js:6: 
On 2011/08/17 03:18:39, James Hawkins wrote:
> Remove blank line.

Done.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options.html (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options.html:164: <include
src="intents_view.html">
On 2011/08/17 03:18:39, James Hawkins wrote:
> Wrap in the pp_ifdef?

Yes. Missed this one.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/ui/intents/inten...
File chrome/browser/ui/intents/intents_model.cc (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/ui/intents/inten...
chrome/browser/ui/intents/intents_model.cc:125: if (intents.size() == 0) {
On 2011/08/17 03:18:39, James Hawkins wrote:
> Remove this block now that we have the registry.

Done.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/ui/intents/inten...
File chrome/browser/ui/intents/intents_model.h (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/ui/intents/inten...
chrome/browser/ui/intents/intents_model.h:45: NodeType type_;
On 2011/08/17 03:18:39, James Hawkins wrote:
> All classes, member vars, methods need documentation.

The class doc talks about this TYPE_* structure notation. Added a comment to
ServiceTreeNode as well. Any particularly puzzling members? They by-and-large
look self-documenting to me.

http://codereview.chromium.org/7624012/diff/1/chrome/browser/ui/intents/inten...
File chrome/browser/ui/intents/intents_model_unittest.cc (right):

http://codereview.chromium.org/7624012/diff/1/chrome/browser/ui/intents/inten...
chrome/browser/ui/intents/intents_model_unittest.cc:17: 
On 2011/08/17 03:18:39, James Hawkins wrote:
> Deadcode.

Now filled in below. (Uploaded this last night before getting to this test.)

http://codereview.chromium.org/7624012/diff/1/content/renderer/render_view.h
File content/renderer/render_view.h (right):

http://codereview.chromium.org/7624012/diff/1/content/renderer/render_view.h#...
content/renderer/render_view.h:443: const WebKit::WebString& type,
On 2011/08/17 03:18:39, James Hawkins wrote:
> This is from the other CL, right?

Yeah, unfortunately it looks like there's no way to base a diff on another
change. All this should merge out.

[James Hawkins, 2011-08-17 21:34:15]

http://codereview.chromium.org/7624012/diff/3003/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7624012/diff/3003/build/common.gypi#newcode918
build/common.gypi:918: 'ENABLE_INTENTS=1',
ENABLE_WEB_INTENTS, though we already have that defined elsewhere?

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
File chrome/browser/resources/options/intents_list.js (right):

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_list.js:5: cr.define('options',
function() {
OK, please add a TODO to refactor this with the cookies view code.  I'm wary
about letting this in without it, but I trust you to take a look at it later.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
File chrome/browser/resources/options/intents_view.css (right):

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_view.css:94: -webkit-margin-start:
11em;
These (-webkit*) go at the top.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_view.css:111: -webkit-box-orient:
horizontal;
-webkit at the top, alphabetize display.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_view.css:127: padding: 2px 5px 2px 5px;
Shorthand notation, HAE.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_view.css:158: 
Remove extra blank line.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_view.css:159: /* styles for the intents
details box */
Styles, add period.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
File chrome/browser/ui/intents/intents_model.cc (right):

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.cc:34: if (node->Type() ==
IntentsTreeNode::TYPE_SERVICE) {
Inconsistently using braces. I'd drop the braces for single-line logic blocks;
however, this last if block can be removed, since it will be handled by the
final return.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.cc:42: if (path_id == "")
path_id.empty()

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

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:50: class ServiceTreeNode : public
IntentsTreeNode {
Can this just be a struct? It seems it's only getters and setters.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:84: class IntentsModel : public
ui::TreeNodeModel<IntentsTreeNode>,
Document the class.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:114: void LoadModel();
These are not self-documenting, please document.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:118: WebIntentsRegistry*
intents_registry_;
Always document all vars. Note ownership.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/intents_settings_handler.cc (right):

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/intents_settings_handler.cc:85: Profile* profile
= Profile::FromWebUI(web_ui_);
Save indentation, return early.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/intents_settings_handler.cc:112: //
TreeModelBeginBatch
What are these comments about?

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/intents_settings_handler.h (right):

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/intents_settings_handler.h:17: class
IntentsSettingsHandler : public OptionsPageUIHandler,
Document the class.

[Greg Billock, 2011-08-17 22:44:02]

http://codereview.chromium.org/7624012/diff/3003/build/common.gypi
File build/common.gypi (right):

http://codereview.chromium.org/7624012/diff/3003/build/common.gypi#newcode918
build/common.gypi:918: 'ENABLE_INTENTS=1',
On 2011/08/17 21:34:15, James Hawkins wrote:
> ENABLE_WEB_INTENTS, though we already have that defined elsewhere?

I don't think so. And you're right. That's better for the webkit flag for sure.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
File chrome/browser/resources/options/intents_list.js (right):

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_list.js:5: cr.define('options',
function() {
On 2011/08/17 21:34:15, James Hawkins wrote:
> OK, please add a TODO to refactor this with the cookies view code.  I'm wary
> about letting this in without it, but I trust you to take a look at it later.

I agree that this needs to be done. I put in the note that they need to be
merged once we understand from UX what this'll look like. My reason for not just
inheriting from it was basically uncertainty about the functionality we'd end up
needing from it, and so a desire to decouple it from messing with the cookies
one until we understand that.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
File chrome/browser/resources/options/intents_view.css (right):

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_view.css:94: -webkit-margin-start:
11em;
On 2011/08/17 21:34:15, James Hawkins wrote:
> These (-webkit*) go at the top.

Done.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_view.css:111: -webkit-box-orient:
horizontal;
On 2011/08/17 21:34:15, James Hawkins wrote:
> -webkit at the top, alphabetize display.

Done.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_view.css:127: padding: 2px 5px 2px 5px;
On 2011/08/17 21:34:15, James Hawkins wrote:
> Shorthand notation, HAE.

Done.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_view.css:158: 
On 2011/08/17 21:34:15, James Hawkins wrote:
> Remove extra blank line.

Done.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_view.css:159: /* styles for the intents
details box */
On 2011/08/17 21:34:15, James Hawkins wrote:
> Styles, add period.

Done.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
File chrome/browser/ui/intents/intents_model.cc (right):

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.cc:34: if (node->Type() ==
IntentsTreeNode::TYPE_SERVICE) {
On 2011/08/17 21:34:15, James Hawkins wrote:
> Inconsistently using braces. I'd drop the braces for single-line logic blocks;
> however, this last if block can be removed, since it will be handled by the
> final return.

Done.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.cc:42: if (path_id == "")
On 2011/08/17 21:34:15, James Hawkins wrote:
> path_id.empty()

Done.

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

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:50: class ServiceTreeNode : public
IntentsTreeNode {
On 2011/08/17 21:34:15, James Hawkins wrote:
> Can this just be a struct? It seems it's only getters and setters.

It needs to fit into the TreeNode<T> system, at least the way I have the model
going right now. Given my understanding of how we want the controls to work, I
think a tree model is a reasonable thing to use, but you're right that it ends
up kind of heavy here.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:84: class IntentsModel : public
ui::TreeNodeModel<IntentsTreeNode>,
On 2011/08/17 21:34:15, James Hawkins wrote:
> Document the class.

Done.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:114: void LoadModel();
On 2011/08/17 21:34:15, James Hawkins wrote:
> These are not self-documenting, please document.

Done.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:118: WebIntentsRegistry*
intents_registry_;
On 2011/08/17 21:34:15, James Hawkins wrote:
> Always document all vars. Note ownership.

Done.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/intents_settings_handler.cc (right):

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/intents_settings_handler.cc:85: Profile* profile
= Profile::FromWebUI(web_ui_);
On 2011/08/17 21:34:15, James Hawkins wrote:
> Save indentation, return early.

Done.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/intents_settings_handler.cc:112: //
TreeModelBeginBatch
On 2011/08/17 21:34:15, James Hawkins wrote:
> What are these comments about?

Fixed to a TODO: there's a batch update optimization here; I'm not sure it's
worth doing at this point.

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/intents_settings_handler.h (right):

http://codereview.chromium.org/7624012/diff/3003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/intents_settings_handler.h:17: class
IntentsSettingsHandler : public OptionsPageUIHandler,
On 2011/08/17 21:34:15, James Hawkins wrote:
> Document the class.

Done.

[James Hawkins, 2011-08-17 22:50:29]

LGTM with nits.

http://codereview.chromium.org/7624012/diff/8005/chrome/browser/resources/opt...
File chrome/browser/resources/options/intents_view.js (right):

http://codereview.chromium.org/7624012/diff/8005/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_view.js:51: chrome.send('loadIntents',
[]);
Remove this last (unnecessary) param.

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

http://codereview.chromium.org/7624012/diff/8005/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:123: // The backing registry. Not
owned.
s/Not owned/Weak pointer/

http://codereview.chromium.org/7624012/diff/8005/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/intents_settings_handler.h (right):

http://codereview.chromium.org/7624012/diff/8005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/intents_settings_handler.h:67:
WebIntentsRegistry* web_intents_registry_;  // not owned
Weak pointer.

[Greg Billock, 2011-08-18 17:36:58]

http://codereview.chromium.org/7624012/diff/8005/chrome/browser/resources/opt...
File chrome/browser/resources/options/intents_view.js (right):

http://codereview.chromium.org/7624012/diff/8005/chrome/browser/resources/opt...
chrome/browser/resources/options/intents_view.js:51: chrome.send('loadIntents',
[]);
On 2011/08/17 22:50:29, James Hawkins wrote:
> Remove this last (unnecessary) param.

Done.

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

http://codereview.chromium.org/7624012/diff/8005/chrome/browser/ui/intents/in...
chrome/browser/ui/intents/intents_model.h:123: // The backing registry. Not
owned.
On 2011/08/17 22:50:29, James Hawkins wrote:
> s/Not owned/Weak pointer/

Done.

http://codereview.chromium.org/7624012/diff/8005/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/intents_settings_handler.h (right):

http://codereview.chromium.org/7624012/diff/8005/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/intents_settings_handler.h:67:
WebIntentsRegistry* web_intents_registry_;  // not owned
On 2011/08/17 22:50:29, James Hawkins wrote:
> Weak pointer.

Done.

[commit-bot: I haz the power, 2011-08-20 05:41:19]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win, revision is 97558, job name
was 7624012-11001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-08-23 20:01:27]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac, revision is 97885, job name
was 7624012-16001 (retry) (retry) (previous was lost) (previous was lost).

[commit-bot: I haz the power, 2011-08-24 00:32:32]

Change committed as 97955