Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10732002:
Upstream rewrite of Instant. (Closed)
|
Created:
8 years, 5 months ago by Jered Modified:
8 years, 4 months ago CC:
chromium-reviews, Shishir, James Su, darin-cc_chromium.org, brettw-cc_chromium.org, sreeram, dominich Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpstream rewrite of Instant.
This massive CL attempts to upstream a rewrite of Chrome Instant which
introduces no new features but may fix bugs. Sreeram undertook this
rewrite because the old implementation was too brittle to extend. The
goal here is to get trunk into a state where we can add new features
from our private branch with minimal diffs.
R=sky@chromium.org
BUG=134865
TEST=Browser test builds and runs on linux.
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Merge test #
Total comments: 17
Patch Set 4 : First round of comments #
Total comments: 38
Patch Set 5 : Another round of comments. #Patch Set 6 : Replace id with instant_url. #
Total comments: 11
Patch Set 7 : Unpatch log in web_contents_impl #
Total comments: 2
Patch Set 8 : Address Sreeram's comments. #Patch Set 9 : Rebase and merge. #Patch Set 10 : Use string16 instead of string. #Patch Set 11 : Fix bad merge. #Patch Set 12 : Try to fix mac and windows. #Patch Set 13 : Try again. #
Total comments: 10
Patch Set 14 : Nits. #Patch Set 15 : Extreme nits. #Messages
Total messages: 72 (0 generated)
Note: by TEST=Builds, I meant that chrome builds. Quite probably the tests do not build at all.
On 2012/06/28 18:35:18, jered1 wrote: > Note: by TEST=Builds, I meant that chrome builds. Quite probably the tests do > not build at all. They won't. I'm working on it.
Have the tests been updated?
On 2012/06/28 19:40:54, sky wrote: > Have the tests been updated? Dominic is on it. I wanted to send this changelist out sooner rather than later to get some more eyes on this implementation, and determine we can actually check it in this way.
On 2012/06/28 19:42:19, jered1 wrote: > On 2012/06/28 19:40:54, sky wrote: > > Have the tests been updated? > > Dominic is on it. I wanted to send this changelist out sooner rather than later > to get some more eyes on this implementation, and determine we can actually > check it in this way. I'm a bit worried that might result in changing this stuff around. I'm going to hold off until the test are in order.
tip to get the tests working: put back the chrome notifications for instant-controller-shown and instant-support-determined. there are convenient points in the instant controller where you can send these notifications from. On Jun 29, 2012 12:06 AM, <dominich@chromium.org> wrote: > On 2012/06/28 18:35:18, jered1 wrote: > >> Note: by TEST=Builds, I meant that chrome builds. Quite probably the >> tests do >> not build at all. >> > > They won't. I'm working on it. > > > https://chromiumcodereview.**appspot.com/10732002/<https://chromiumcodereview... >
On 2012/06/28 22:04:56, sreeram wrote: > tip to get the tests working: put back the chrome notifications for > instant-controller-shown and instant-support-determined. there are > convenient points in the instant controller where you can send these > notifications from. I was intending to add an Observer interface to InstantController that would replace the notifications. As I understand it, this would be lower cost than sending notifications, especially when only tests use them. > On Jun 29, 2012 12:06 AM, <mailto:dominich@chromium.org> wrote: > > > On 2012/06/28 18:35:18, jered1 wrote: > > > >> Note: by TEST=Builds, I meant that chrome builds. Quite probably the > >> tests do > >> not build at all. > >> > > > > They won't. I'm working on it. > > > > > > > https://chromiumcodereview.**appspot.com/10732002/%3Chttps://chromiumcoderevi...> > >
On Mon, Jul 2, 2012 at 10:46 PM, <dominich@chromium.org> wrote: > On 2012/06/28 22:04:56, sreeram wrote: >> >> tip to get the tests working: put back the chrome notifications for >> instant-controller-shown and instant-support-determined. there are >> convenient points in the instant controller where you can send these >> notifications from. > > I was intending to add an Observer interface to InstantController that would > replace the notifications. As I understand it, this would be lower cost than > sending notifications, especially when only tests use them. We already send out notifications today that are used for more than tests ("updated" for RLZ tracking and "committed" for GoogleURLTracker), so I think it's simpler to just re-add the notifications that were already there (before I took them out in the refactor).
On 2012/07/03 00:57:49, sreeram wrote:
> On Mon, Jul 2, 2012 at 10:46 PM, <mailto:dominich@chromium.org> wrote:
> > On 2012/06/28 22:04:56, sreeram wrote:
> >>
> >> tip to get the tests working: put back the chrome notifications for
> >> instant-controller-shown and instant-support-determined. there are
> >> convenient points in the instant controller where you can send these
> >> notifications from.
> >
> > I was intending to add an Observer interface to InstantController that would
> > replace the notifications. As I understand it, this would be lower cost than
> > sending notifications, especially when only tests use them.
>
> We already send out notifications today that are used for more than
> tests ("updated" for RLZ tracking and "committed" for
> GoogleURLTracker), so I think it's simpler to just re-add the
> notifications that were already there (before I took them out in the
> refactor).
It looks like this CL doesn't remove the notification definitions, so agreed.
I'll just hook those back up.
On 2012/07/03 21:10:57, dominich wrote:
> On 2012/07/03 00:57:49, sreeram wrote:
> > On Mon, Jul 2, 2012 at 10:46 PM, <mailto:dominich@chromium.org> wrote:
> > > On 2012/06/28 22:04:56, sreeram wrote:
> > >>
> > >> tip to get the tests working: put back the chrome notifications for
> > >> instant-controller-shown and instant-support-determined. there are
> > >> convenient points in the instant controller where you can send these
> > >> notifications from.
> > >
> > > I was intending to add an Observer interface to InstantController that
would
> > > replace the notifications. As I understand it, this would be lower cost
than
> > > sending notifications, especially when only tests use them.
> >
> > We already send out notifications today that are used for more than
> > tests ("updated" for RLZ tracking and "committed" for
> > GoogleURLTracker), so I think it's simpler to just re-add the
> > notifications that were already there (before I took them out in the
> > refactor).
>
> It looks like this CL doesn't remove the notification definitions, so agreed.
> I'll just hook those back up.
Dominic has a new CL with updated tests and a couple of fixes.
Is this patch going to be closed out then? -Scott On Mon, Jul 16, 2012 at 9:08 AM, <jered@chromium.org> wrote: > On 2012/07/03 21:10:57, dominich wrote: >> >> On 2012/07/03 00:57:49, sreeram wrote: >> > On Mon, Jul 2, 2012 at 10:46 PM, <mailto:dominich@chromium.org> wrote: >> > > On 2012/06/28 22:04:56, sreeram wrote: >> > >> >> > >> tip to get the tests working: put back the chrome notifications for >> > >> instant-controller-shown and instant-support-determined. there are >> > >> convenient points in the instant controller where you can send these >> > >> notifications from. >> > > >> > > I was intending to add an Observer interface to InstantController that > > would >> >> > > replace the notifications. As I understand it, this would be lower >> > > cost > > than >> >> > > sending notifications, especially when only tests use them. >> > >> > We already send out notifications today that are used for more than >> > tests ("updated" for RLZ tracking and "committed" for >> > GoogleURLTracker), so I think it's simpler to just re-add the >> > notifications that were already there (before I took them out in the >> > refactor). > > >> It looks like this CL doesn't remove the notification definitions, so >> agreed. >> I'll just hook those back up. > > > Dominic has a new CL with updated tests and a couple of fixes. > > https://chromiumcodereview.appspot.com/10732002/
No, Dominic's CL is against this patch. Sorry for the confusion. On Mon, Jul 16, 2012 at 9:10 AM, Scott Violet <sky@chromium.org> wrote: > Is this patch going to be closed out then? > > -Scott > > On Mon, Jul 16, 2012 at 9:08 AM, <jered@chromium.org> wrote: > > On 2012/07/03 21:10:57, dominich wrote: > >> > >> On 2012/07/03 00:57:49, sreeram wrote: > >> > On Mon, Jul 2, 2012 at 10:46 PM, <mailto:dominich@chromium.org> > wrote: > >> > > On 2012/06/28 22:04:56, sreeram wrote: > >> > >> > >> > >> tip to get the tests working: put back the chrome notifications for > >> > >> instant-controller-shown and instant-support-determined. there are > >> > >> convenient points in the instant controller where you can send > these > >> > >> notifications from. > >> > > > >> > > I was intending to add an Observer interface to InstantController > that > > > > would > >> > >> > > replace the notifications. As I understand it, this would be lower > >> > > cost > > > > than > >> > >> > > sending notifications, especially when only tests use them. > >> > > >> > We already send out notifications today that are used for more than > >> > tests ("updated" for RLZ tracking and "committed" for > >> > GoogleURLTracker), so I think it's simpler to just re-add the > >> > notifications that were already there (before I took them out in the > >> > refactor). > > > > > >> It looks like this CL doesn't remove the notification definitions, so > >> agreed. > >> I'll just hook those back up. > > > > > > Dominic has a new CL with updated tests and a couple of fixes. > > > > https://chromiumcodereview.appspot.com/10732002/ >
Since we can't land a patch that'll break tests please merge them. -Scott On Mon, Jul 16, 2012 at 9:29 AM, Jered Wierzbicki <jered@google.com> wrote: > No, Dominic's CL is against this patch. > Sorry for the confusion. > > > On Mon, Jul 16, 2012 at 9:10 AM, Scott Violet <sky@chromium.org> wrote: >> >> Is this patch going to be closed out then? >> >> -Scott >> >> On Mon, Jul 16, 2012 at 9:08 AM, <jered@chromium.org> wrote: >> > On 2012/07/03 21:10:57, dominich wrote: >> >> >> >> On 2012/07/03 00:57:49, sreeram wrote: >> >> > On Mon, Jul 2, 2012 at 10:46 PM, <mailto:dominich@chromium.org> >> >> > wrote: >> >> > > On 2012/06/28 22:04:56, sreeram wrote: >> >> > >> >> >> > >> tip to get the tests working: put back the chrome notifications >> >> > >> for >> >> > >> instant-controller-shown and instant-support-determined. there are >> >> > >> convenient points in the instant controller where you can send >> >> > >> these >> >> > >> notifications from. >> >> > > >> >> > > I was intending to add an Observer interface to InstantController >> >> > > that >> > >> > would >> >> >> >> > > replace the notifications. As I understand it, this would be lower >> >> > > cost >> > >> > than >> >> >> >> > > sending notifications, especially when only tests use them. >> >> > >> >> > We already send out notifications today that are used for more than >> >> > tests ("updated" for RLZ tracking and "committed" for >> >> > GoogleURLTracker), so I think it's simpler to just re-add the >> >> > notifications that were already there (before I took them out in the >> >> > refactor). >> > >> > >> >> It looks like this CL doesn't remove the notification definitions, so >> >> agreed. >> >> I'll just hook those back up. >> > >> > >> > Dominic has a new CL with updated tests and a couple of fixes. >> > >> > https://chromiumcodereview.appspot.com/10732002/ > >
On 2012/07/16 16:35:41, sky wrote:
> Since we can't land a patch that'll break tests please merge them.
Done.
>
> -Scott
>
> On Mon, Jul 16, 2012 at 9:29 AM, Jered Wierzbicki <mailto:jered@google.com>
wrote:
> > No, Dominic's CL is against this patch.
> > Sorry for the confusion.
> >
> >
> > On Mon, Jul 16, 2012 at 9:10 AM, Scott Violet <mailto:sky@chromium.org>
wrote:
> >>
> >> Is this patch going to be closed out then?
> >>
> >> -Scott
> >>
> >> On Mon, Jul 16, 2012 at 9:08 AM, <mailto:jered@chromium.org> wrote:
> >> > On 2012/07/03 21:10:57, dominich wrote:
> >> >>
> >> >> On 2012/07/03 00:57:49, sreeram wrote:
> >> >> > On Mon, Jul 2, 2012 at 10:46 PM, <mailto:dominich@chromium.org>
> >> >> > wrote:
> >> >> > > On 2012/06/28 22:04:56, sreeram wrote:
> >> >> > >>
> >> >> > >> tip to get the tests working: put back the chrome notifications
> >> >> > >> for
> >> >> > >> instant-controller-shown and instant-support-determined. there are
> >> >> > >> convenient points in the instant controller where you can send
> >> >> > >> these
> >> >> > >> notifications from.
> >> >> > >
> >> >> > > I was intending to add an Observer interface to InstantController
> >> >> > > that
> >> >
> >> > would
> >> >>
> >> >> > > replace the notifications. As I understand it, this would be lower
> >> >> > > cost
> >> >
> >> > than
> >> >>
> >> >> > > sending notifications, especially when only tests use them.
> >> >> >
> >> >> > We already send out notifications today that are used for more than
> >> >> > tests ("updated" for RLZ tracking and "committed" for
> >> >> > GoogleURLTracker), so I think it's simpler to just re-add the
> >> >> > notifications that were already there (before I took them out in the
> >> >> > refactor).
> >> >
> >> >
> >> >> It looks like this CL doesn't remove the notification definitions, so
> >> >> agreed.
> >> >> I'll just hook those back up.
> >> >
> >> >
> >> > Dominic has a new CL with updated tests and a couple of fixes.
> >> >
> >> > https://chromiumcodereview.appspot.com/10732002/
> >
> >
I only made it little bit through this review as I started to encounter some show stoppers. From reviewing large changes in the past this will go much smoother if you break things up as much as possible. For example, the search_box changes aren't dependent on the rest of this and could land. You could also remove the ability to load arbitrary urls from InstantLoader... http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... File chrome/browser/instant/instant_loader.cc (left): http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... chrome/browser/instant/instant_loader.cc:68: const int kUpdateBoundsDelayMS = 1000; I guess we're removing this because it doesn't makes sense with the new behavior, right? But remember that we need to support both for at least some amount of time. So, I don't think we can remove this. http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... chrome/browser/instant/instant_loader.cc:71: const int kHostBlacklistStatusCode = 403; Why are you removing this? We've documented allowing this for the API and folks on the search side wanted this as a way to disable instant if necessary. http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... chrome/browser/instant/instant_loader.cc:104: const char* const InstantLoader::kInstantHeaderValue = "instant"; Similar comments for the header and header-value. These are the in spec and while Google may not want them, they should be there. http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... File chrome/browser/instant/instant_loader.h (right): http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... chrome/browser/instant/instant_loader.h:37: #if defined(OS_MACOSX) All the mac specific ifdefs make the code hard to read. Remove them and instead ifdef the contents of the observe implementation. http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... chrome/browser/instant/instant_loader.h:43: // is the page the preview will be shown on top of and potentially replace. Document loader_id. http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... chrome/browser/instant/instant_loader.h:71: string16 loader_id() const { return loader_id_; } const string16& http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... chrome/browser/instant/instant_loader.h:74: // non-NULL until |ReleasePreviewContents| is called. |'s are only used for fields/arguments. Use () for functions, eg ReleasePreviewContents().
I think I can have a go at splitting off the searchbox change, but will need to wait until Sreeram returns for him to justify the more significant (spec-visible) changes here. On Tue, Jul 17, 2012 at 11:21 AM, <sky@chromium.org> wrote: > I only made it little bit through this review as I started to encounter > some > show stoppers. > > From reviewing large changes in the past this will go much smoother if you > break > things up as much as possible. For example, the search_box changes aren't > dependent on the rest of this and could land. You could also remove the > ability > to load arbitrary urls from InstantLoader... > > > http://chromiumcodereview.**appspot.com/10732002/diff/** > 11002/chrome/browser/instant/**instant_loader.cc<http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/instant/instant_loader.cc> > File chrome/browser/instant/**instant_loader.cc (left): > > http://chromiumcodereview.**appspot.com/10732002/diff/** > 11002/chrome/browser/instant/**instant_loader.cc#oldcode68<http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/instant/instant_loader.cc#oldcode68> > chrome/browser/instant/**instant_loader.cc:68: const int > kUpdateBoundsDelayMS = 1000; > I guess we're removing this because it doesn't makes sense with the new > behavior, right? But remember that we need to support both for at least > some amount of time. So, I don't think we can remove this. > > http://chromiumcodereview.**appspot.com/10732002/diff/** > 11002/chrome/browser/instant/**instant_loader.cc#oldcode71<http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/instant/instant_loader.cc#oldcode71> > chrome/browser/instant/**instant_loader.cc:71: const int > kHostBlacklistStatusCode = 403; > Why are you removing this? We've documented allowing this for the API > and folks on the search side wanted this as a way to disable instant if > necessary. > > http://chromiumcodereview.**appspot.com/10732002/diff/** > 11002/chrome/browser/instant/**instant_loader.cc#oldcode104<http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/instant/instant_loader.cc#oldcode104> > chrome/browser/instant/**instant_loader.cc:104: const char* const > InstantLoader::**kInstantHeaderValue = "instant"; > Similar comments for the header and header-value. These are the in spec > and while Google may not want them, they should be there. > > http://chromiumcodereview.**appspot.com/10732002/diff/** > 11002/chrome/browser/instant/**instant_loader.h<http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/instant/instant_loader.h> > File chrome/browser/instant/**instant_loader.h (right): > > http://chromiumcodereview.**appspot.com/10732002/diff/** > 11002/chrome/browser/instant/**instant_loader.h#newcode37<http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/instant/instant_loader.h#newcode37> > chrome/browser/instant/**instant_loader.h:37: #if defined(OS_MACOSX) > All the mac specific ifdefs make the code hard to read. Remove them and > instead ifdef the contents of the observe implementation. > > http://chromiumcodereview.**appspot.com/10732002/diff/** > 11002/chrome/browser/instant/**instant_loader.h#newcode43<http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/instant/instant_loader.h#newcode43> > chrome/browser/instant/**instant_loader.h:43: // is the page the preview > will be shown on top of and potentially replace. > Document loader_id. > > http://chromiumcodereview.**appspot.com/10732002/diff/** > 11002/chrome/browser/instant/**instant_loader.h#newcode71<http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/instant/instant_loader.h#newcode71> > chrome/browser/instant/**instant_loader.h:71: string16 loader_id() const { > return loader_id_; } > const string16& > > http://chromiumcodereview.**appspot.com/10732002/diff/** > 11002/chrome/browser/instant/**instant_loader.h#newcode74<http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/instant/instant_loader.h#newcode74> > chrome/browser/instant/**instant_loader.h:74: // non-NULL until > |ReleasePreviewContents| is called. > |'s are only used for fields/arguments. Use () for functions, eg > ReleasePreviewContents(). > > http://chromiumcodereview.**appspot.com/10732002/<http://chromiumcodereview.a... >
http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader.cc (left): http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:104: const char* const InstantLoader::kInstantHeaderValue = "instant"; On 2012/07/17 18:21:42, sky wrote: > Similar comments for the header and header-value. These are the in spec and > while Google may not want them, they should be there. They are - see 262. They were removed as constants but they're still being sent.
On Tue, Jul 17, 2012 at 11:42 AM, <dominich@chromium.org> wrote: > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > File chrome/browser/instant/instant_loader.cc (left): > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > chrome/browser/instant/instant_loader.cc:104: const char* const > InstantLoader::kInstantHeaderValue = "instant"; > On 2012/07/17 18:21:42, sky wrote: >> >> Similar comments for the header and header-value. These are the in > > spec and >> >> while Google may not want them, they should be there. > > > They are - see 262. > > They were removed as constants but they're still being sent. > > http://codereview.chromium.org/10732002/ Ah, ok. I see them. Make a new constant for these then. -Scott
You can also nuke the arbitrary url loading from InstantLoader now. -Scott On Tue, Jul 17, 2012 at 11:32 AM, Jered Wierzbicki <jered@google.com> wrote: > I think I can have a go at splitting off the searchbox change, but will need > to wait until Sreeram returns for him to justify the more significant > (spec-visible) > changes here. > > > On Tue, Jul 17, 2012 at 11:21 AM, <sky@chromium.org> wrote: >> >> I only made it little bit through this review as I started to encounter >> some >> show stoppers. >> >> From reviewing large changes in the past this will go much smoother if you >> break >> things up as much as possible. For example, the search_box changes aren't >> dependent on the rest of this and could land. You could also remove the >> ability >> to load arbitrary urls from InstantLoader... >> >> >> >> http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... >> File chrome/browser/instant/instant_loader.cc (left): >> >> >> http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... >> chrome/browser/instant/instant_loader.cc:68: const int >> kUpdateBoundsDelayMS = 1000; >> I guess we're removing this because it doesn't makes sense with the new >> behavior, right? But remember that we need to support both for at least >> some amount of time. So, I don't think we can remove this. >> >> >> http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... >> chrome/browser/instant/instant_loader.cc:71: const int >> kHostBlacklistStatusCode = 403; >> Why are you removing this? We've documented allowing this for the API >> and folks on the search side wanted this as a way to disable instant if >> necessary. >> >> >> http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... >> chrome/browser/instant/instant_loader.cc:104: const char* const >> InstantLoader::kInstantHeaderValue = "instant"; >> Similar comments for the header and header-value. These are the in spec >> and while Google may not want them, they should be there. >> >> >> http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... >> File chrome/browser/instant/instant_loader.h (right): >> >> >> http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... >> chrome/browser/instant/instant_loader.h:37: #if defined(OS_MACOSX) >> All the mac specific ifdefs make the code hard to read. Remove them and >> instead ifdef the contents of the observe implementation. >> >> >> http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... >> chrome/browser/instant/instant_loader.h:43: // is the page the preview >> will be shown on top of and potentially replace. >> Document loader_id. >> >> >> http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... >> chrome/browser/instant/instant_loader.h:71: string16 loader_id() const { >> return loader_id_; } >> const string16& >> >> >> http://chromiumcodereview.appspot.com/10732002/diff/11002/chrome/browser/inst... >> chrome/browser/instant/instant_loader.h:74: // non-NULL until >> |ReleasePreviewContents| is called. >> |'s are only used for fields/arguments. Use () for functions, eg >> ReleasePreviewContents(). >> >> http://chromiumcodereview.appspot.com/10732002/ > >
http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader.cc (left): http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:68: const int kUpdateBoundsDelayMS = 1000; On 2012/07/17 18:21:42, sky wrote: > I guess we're removing this because it doesn't makes sense with the new > behavior, right? But remember that we need to support both for at least some > amount of time. So, I don't think we can remove this. It's not being removed. It's just being moved to instant_controller.cc (line 56), which centralizes the logic for when to update the bounds. http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:68: const int kUpdateBoundsDelayMS = 1000; On 2012/07/17 18:21:42, sky wrote: > I guess we're removing this because it doesn't makes sense with the new > behavior, right? But remember that we need to support both for at least some > amount of time. So, I don't think we can remove this. It's not being removed. It's just being moved to instant_controller.cc (line 56). http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:71: const int kHostBlacklistStatusCode = 403; On 2012/07/17 18:21:42, sky wrote: > Why are you removing this? We've documented allowing this for the API and folks > on the search side wanted this as a way to disable instant if necessary. I don't see it being documented at http://dev.chromium.org/searchbox. The search folks already have and use another mechanism for disabling Instant, which is to simply not define window.chrome.sv (v1 API) or window.chrome.searchbox.onsubmit (v2 API). It has exactly the same effect as returning 403 with respect to not using and blacklisting the search provider for Instant. None of these are also documented, by the way.
On Tue, Jul 17, 2012 at 6:29 PM, <sreeram@chromium.org> wrote: > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > File chrome/browser/instant/instant_loader.cc (left): > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > chrome/browser/instant/instant_loader.cc:68: const int > kUpdateBoundsDelayMS = 1000; > On 2012/07/17 18:21:42, sky wrote: >> >> I guess we're removing this because it doesn't makes sense with the > > new >> >> behavior, right? But remember that we need to support both for at > > least some >> >> amount of time. So, I don't think we can remove this. > > > It's not being removed. It's just being moved to instant_controller.cc > (line 56), which centralizes the logic for when to update the bounds. > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > chrome/browser/instant/instant_loader.cc:68: const int > kUpdateBoundsDelayMS = 1000; > On 2012/07/17 18:21:42, sky wrote: >> >> I guess we're removing this because it doesn't makes sense with the > > new >> >> behavior, right? But remember that we need to support both for at > > least some >> >> amount of time. So, I don't think we can remove this. > > > It's not being removed. It's just being moved to instant_controller.cc > (line 56). > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > chrome/browser/instant/instant_loader.cc:71: const int > kHostBlacklistStatusCode = 403; > On 2012/07/17 18:21:42, sky wrote: >> >> Why are you removing this? We've documented allowing this for the API > > and folks >> >> on the search side wanted this as a way to disable instant if > > necessary. > > I don't see it being documented at http://dev.chromium.org/searchbox. > The search folks already have and use another mechanism for disabling > Instant, which is to simply not define window.chrome.sv (v1 API) or > window.chrome.searchbox.onsubmit (v2 API). It has exactly the same > effect as returning 403 with respect to not using and blacklisting the > search provider for Instant. None of these are also documented, by the > way. > > http://codereview.chromium.org/10732002/ Great to hear! -Scott
I did not understand your comments about loading arbitrary urls. Is this a proposal for a way to split out more of this work into a smaller CL? Could you please explain in more detail what should be done there? Thanks. http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader.cc (left): http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:68: const int kUpdateBoundsDelayMS = 1000; On 2012/07/17 18:21:42, sky wrote: > I guess we're removing this because it doesn't makes sense with the new > behavior, right? But remember that we need to support both for at least some > amount of time. So, I don't think we can remove this. This still exists, but is now in the controller. http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:104: const char* const InstantLoader::kInstantHeaderValue = "instant"; On 2012/07/17 18:21:42, sky wrote: > Similar comments for the header and header-value. These are the in spec and > while Google may not want them, they should be there. I have re-added these below. I chose not to expose them because they're an implementation detail of the loader. http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader.h (right): http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.h:37: #if defined(OS_MACOSX) On 2012/07/17 18:21:42, sky wrote: > All the mac specific ifdefs make the code hard to read. Remove them and instead > ifdef the contents of the observe implementation. Done. http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.h:43: // is the page the preview will be shown on top of and potentially replace. On 2012/07/17 18:21:42, sky wrote: > Document loader_id. Done. http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.h:71: string16 loader_id() const { return loader_id_; } On 2012/07/17 18:21:42, sky wrote: > const string16& Done. http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.h:74: // non-NULL until |ReleasePreviewContents| is called. On 2012/07/17 18:21:42, sky wrote: > |'s are only used for fields/arguments. Use () for functions, eg > ReleasePreviewContents(). Done.
On Wed, Jul 18, 2012 at 10:38 AM, <jered@chromium.org> wrote: > I did not understand your comments about loading arbitrary urls. Is this a > proposal for a way to split out more of this work into a smaller CL? Could > you > please explain in more detail what should be done there? Thanks. See InstantLoader::Update(). It takes an arbitrary url to load now. And yes, this was a suggestion as to a way to break the view into chunks. -Scott > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > File chrome/browser/instant/instant_loader.cc (left): > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > chrome/browser/instant/instant_loader.cc:68: const int > kUpdateBoundsDelayMS = 1000; > On 2012/07/17 18:21:42, sky wrote: >> >> I guess we're removing this because it doesn't makes sense with the > > new >> >> behavior, right? But remember that we need to support both for at > > least some >> >> amount of time. So, I don't think we can remove this. > > > This still exists, but is now in the controller. > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > chrome/browser/instant/instant_loader.cc:104: const char* const > InstantLoader::kInstantHeaderValue = "instant"; > On 2012/07/17 18:21:42, sky wrote: >> >> Similar comments for the header and header-value. These are the in > > spec and >> >> while Google may not want them, they should be there. > > > I have re-added these below. I chose not to expose them because they're > an implementation detail of the loader. > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > File chrome/browser/instant/instant_loader.h (right): > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > chrome/browser/instant/instant_loader.h:37: #if defined(OS_MACOSX) > > On 2012/07/17 18:21:42, sky wrote: >> >> All the mac specific ifdefs make the code hard to read. Remove them > > and instead >> >> ifdef the contents of the observe implementation. > > > Done. > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > chrome/browser/instant/instant_loader.h:43: // is the page the preview > will be shown on top of and potentially replace. > On 2012/07/17 18:21:42, sky wrote: >> >> Document loader_id. > > > Done. > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > chrome/browser/instant/instant_loader.h:71: string16 loader_id() const { > return loader_id_; } > On 2012/07/17 18:21:42, sky wrote: >> >> const string16& > > > Done. > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > chrome/browser/instant/instant_loader.h:74: // non-NULL until > |ReleasePreviewContents| is called. > On 2012/07/17 18:21:42, sky wrote: >> >> |'s are only used for fields/arguments. Use () for functions, eg >> ReleasePreviewContents(). > > > Done. > > http://codereview.chromium.org/10732002/
On 2012/07/18 20:52:47, sky wrote: > On Wed, Jul 18, 2012 at 10:38 AM, <mailto:jered@chromium.org> wrote: > > I did not understand your comments about loading arbitrary urls. Is this a > > proposal for a way to split out more of this work into a smaller CL? Could > > you > > please explain in more detail what should be done there? Thanks. > > See InstantLoader::Update(). It takes an arbitrary url to load now. I am still confused. This is what I think you mean: In the current state (i.e. "now"), there is one InstantLoader which lives forever, and Update may cause it to load a new URL. In the refactor, we create a new loader per URL. Do this without other changes. I think this is responsible for most of the changes in the present CL and is not easily separable. I could be wrong about that, though. I'll give it a try. Please let me know if I've misunderstood. > And yes, this was a suggestion as to a way to break the view into > chunks. > > -Scott > > > > > > > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > File chrome/browser/instant/instant_loader.cc (left): > > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > chrome/browser/instant/instant_loader.cc:68: const int > > kUpdateBoundsDelayMS = 1000; > > On 2012/07/17 18:21:42, sky wrote: > >> > >> I guess we're removing this because it doesn't makes sense with the > > > > new > >> > >> behavior, right? But remember that we need to support both for at > > > > least some > >> > >> amount of time. So, I don't think we can remove this. > > > > > > This still exists, but is now in the controller. > > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > > > chrome/browser/instant/instant_loader.cc:104: const char* const > > InstantLoader::kInstantHeaderValue = "instant"; > > On 2012/07/17 18:21:42, sky wrote: > >> > >> Similar comments for the header and header-value. These are the in > > > > spec and > >> > >> while Google may not want them, they should be there. > > > > > > I have re-added these below. I chose not to expose them because they're > > an implementation detail of the loader. > > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > File chrome/browser/instant/instant_loader.h (right): > > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > chrome/browser/instant/instant_loader.h:37: #if defined(OS_MACOSX) > > > > On 2012/07/17 18:21:42, sky wrote: > >> > >> All the mac specific ifdefs make the code hard to read. Remove them > > > > and instead > >> > >> ifdef the contents of the observe implementation. > > > > > > Done. > > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > > > chrome/browser/instant/instant_loader.h:43: // is the page the preview > > will be shown on top of and potentially replace. > > On 2012/07/17 18:21:42, sky wrote: > >> > >> Document loader_id. > > > > > > Done. > > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > > > chrome/browser/instant/instant_loader.h:71: string16 loader_id() const { > > return loader_id_; } > > On 2012/07/17 18:21:42, sky wrote: > >> > >> const string16& > > > > > > Done. > > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... > > > > chrome/browser/instant/instant_loader.h:74: // non-NULL until > > |ReleasePreviewContents| is called. > > On 2012/07/17 18:21:42, sky wrote: > >> > >> |'s are only used for fields/arguments. Use () for functions, eg > >> ReleasePreviewContents(). > > > > > > Done. > > > > http://codereview.chromium.org/10732002/
On Wed, Jul 18, 2012 at 3:15 PM, <jered@chromium.org> wrote: > On 2012/07/18 20:52:47, sky wrote: > >> On Wed, Jul 18, 2012 at 10:38 AM, <mailto:jered@chromium.org> wrote: >> > I did not understand your comments about loading arbitrary urls. Is this >> > a >> > proposal for a way to split out more of this work into a smaller CL? >> > Could >> > you >> > please explain in more detail what should be done there? Thanks. > > >> See InstantLoader::Update(). It takes an arbitrary url to load now. > > > I am still confused. This is what I think you mean: > In the current state (i.e. "now"), there is one InstantLoader which lives > forever, > and Update may cause it to load a new URL. In the refactor, we create a new > loader per URL. Do this without other changes. On trunk InstantLoader supports two distinct behaviors: loading urls that have an associated TemplateURL (search urls) and loading non-search urls (say nytimes.com). The later behavior exists from an early version of instant and is no longer used. > I think this is responsible for most of the changes in the present CL and is > not > easily separable. I could be wrong about that, though. I'll give it a try. > Please let > me know if I've misunderstood. It may be the case that it isn't easy to separate this out. I'll take a look at the patch again. -Scott > >> And yes, this was a suggestion as to a way to break the view into >> chunks. > > >> -Scott > > >> > >> > >> > >> > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... >> >> > File chrome/browser/instant/instant_loader.cc (left): >> > >> > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... >> >> > chrome/browser/instant/instant_loader.cc:68: const int >> > kUpdateBoundsDelayMS = 1000; >> > On 2012/07/17 18:21:42, sky wrote: >> >> >> >> I guess we're removing this because it doesn't makes sense with the >> > >> > new >> >> >> >> behavior, right? But remember that we need to support both for at >> > >> > least some >> >> >> >> amount of time. So, I don't think we can remove this. >> > >> > >> > This still exists, but is now in the controller. >> > >> > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... >> >> > >> > chrome/browser/instant/instant_loader.cc:104: const char* const >> > InstantLoader::kInstantHeaderValue = "instant"; >> > On 2012/07/17 18:21:42, sky wrote: >> >> >> >> Similar comments for the header and header-value. These are the in >> > >> > spec and >> >> >> >> while Google may not want them, they should be there. >> > >> > >> > I have re-added these below. I chose not to expose them because they're >> > an implementation detail of the loader. >> > >> > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... >> >> > File chrome/browser/instant/instant_loader.h (right): >> > >> > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... >> >> > chrome/browser/instant/instant_loader.h:37: #if defined(OS_MACOSX) >> > >> > On 2012/07/17 18:21:42, sky wrote: >> >> >> >> All the mac specific ifdefs make the code hard to read. Remove them >> > >> > and instead >> >> >> >> ifdef the contents of the observe implementation. >> > >> > >> > Done. >> > >> > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... >> >> > >> > chrome/browser/instant/instant_loader.h:43: // is the page the preview >> > will be shown on top of and potentially replace. >> > On 2012/07/17 18:21:42, sky wrote: >> >> >> >> Document loader_id. >> > >> > >> > Done. >> > >> > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... >> >> > >> > chrome/browser/instant/instant_loader.h:71: string16 loader_id() const { >> > return loader_id_; } >> > On 2012/07/17 18:21:42, sky wrote: >> >> >> >> const string16& >> > >> > >> > Done. >> > >> > > > > http://codereview.chromium.org/10732002/diff/11002/chrome/browser/instant/ins... >> >> > >> > chrome/browser/instant/instant_loader.h:74: // non-NULL until >> > |ReleasePreviewContents| is called. >> > On 2012/07/17 18:21:42, sky wrote: >> >> >> >> |'s are only used for fields/arguments. Use () for functions, eg >> >> ReleasePreviewContents(). >> > >> > >> > Done. >> > >> > http://codereview.chromium.org/10732002/ > > > > > http://codereview.chromium.org/10732002/
http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.cc (left): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:266: gfx::NativeView view_gaining_focus) { Why is it safe to nuke all this code? http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.cc (right): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:50: PREVIEW_NUM_TYPES, If you're going to have NUM_TYPES, explicitly set CREATED to 0. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:152: if (!tab_contents) { It doesn't make sense to have a DCHECK then a conditional. If GetInstantHostTabContents() never returns NULL, then a DCHECK and no if. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:183: // call |Update| again, at which time we'll update last_full_text_. |last_full_text_| http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:270: DeleteLoader(); Add a comment as to why DeleteLoader is invoked after the DeleteSoon call. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:483: *instant_url = instant_url_ref.SupportsReplacement() ? If the instant_url_ref doesn't support replacement this should return false. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:29: // |InstantController| maintains a WebContents that is intended to give a Use |s for members/arguments, () for functions and generally nothing for names. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:114: WARN_UNUSED_RESULT; Wrap 'InstantCommitType type' so that WARN_UNUSED_RESULT isn't dangling (similar to how const is treated). http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:157: // If |template_url| is a valid TemplateURL for use with Instant, fills in Description should include kInstantURL. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:204: bool loader_ready_; This name is confusing. Its not whether the loader is ready, rather the loader has processed the last request. Maybe loader_processed_last_update_. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:220: std::map<string16, int> blacklisted_ids_; This assumes that once it sees a keyword the TemplateURL associated with it never changes. That is wrong. There is nothing stopping the user from manipulating the set of keywords in various ways during the session. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader.cc (right): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:113: // If this is being called, something is swapping in to our preview_contents_ |preview_contents_| http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:260: SetupPreviewContents(); These are expensive operations and should be moved to an Init methods. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:290: const std::string& text) { This should take string16. If it needs to convert to utf8 for the messages, do it here. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader_delegate.h (right): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_loader_delegate.h:24: InstantLoader* loader, Passing in the loader was really only necessary when multiple loaders were used. Since we don't need that functionality anymore remove the loader argument from all of these. http://codereview.chromium.org/10732002/diff/19001/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10732002/diff/19001/chrome/common/instant_type... chrome/common/instant_types.h:15: // Autocomplete the suggestion after a delay. Its my understanding we're not using this, can it be removed now?
http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader_delegate.h (right): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_loader_delegate.h:24: InstantLoader* loader, On 2012/07/19 17:55:19, sky wrote: > Passing in the loader was really only necessary when multiple loaders were used. > Since we don't need that functionality anymore remove the loader argument from > all of these. There will be other loaders in the future. For instance, at some point we will want to have a URL Loader that shows previews of URLs. As such, these are still important to have.
On 2012/07/19 18:03:43, dominich wrote: > http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... > File chrome/browser/instant/instant_loader_delegate.h (right): > > http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... > chrome/browser/instant/instant_loader_delegate.h:24: InstantLoader* loader, > On 2012/07/19 17:55:19, sky wrote: > > Passing in the loader was really only necessary when multiple loaders were > used. > > Since we don't need that functionality anymore remove the loader argument from > > all of these. > > There will be other loaders in the future. For instance, at some point we will > want to have a URL Loader that shows previews of URLs. As such, these are still > important to have. If that's that case, then why are we ripping out the url loading feature and lots of the other stuff in InstantLoader? For example, waiting for a paint, listening for 403...
On Thu, Jul 19, 2012 at 12:35 PM, <sky@chromium.org> wrote: > On 2012/07/19 18:03:43, dominich wrote: > > http://codereview.chromium.**org/10732002/diff/19001/** > chrome/browser/instant/**instant_loader_delegate.h<http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/instant_loader_delegate.h> > >> File chrome/browser/instant/**instant_loader_delegate.h (right): >> > > > http://codereview.chromium.**org/10732002/diff/19001/** > chrome/browser/instant/**instant_loader_delegate.h#**newcode24<http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/instant_loader_delegate.h#newcode24> > >> chrome/browser/instant/**instant_loader_delegate.h:24: InstantLoader* >> loader, >> On 2012/07/19 17:55:19, sky wrote: >> > Passing in the loader was really only necessary when multiple loaders >> were >> used. >> > Since we don't need that functionality anymore remove the loader >> argument >> > from > >> > all of these. >> > > There will be other loaders in the future. For instance, at some point we >> will >> want to have a URL Loader that shows previews of URLs. As such, these are >> > still > >> important to have. >> > > If that's that case, then why are we ripping out the url loading feature > and > lots of the other stuff in InstantLoader? For example, waiting for a paint, > listening for 403... > The URL loading in Instant was poorly implemented and doesn't take into account many of the issues encountered when trying to preview URLs that the user hasn't explicitly navigated to. We have a version coming that is based on Prerendering that does take those things into account - it is implemented as a different type of InstantLoader. This keeps the code much cleaner and allows smooth interplay between search instant and url instant. Ie, this CL should be considered as abandoning URL instant. A future CL will (if it lands ;)) reintroduce URL instant in a much cleaner way. To do so, we need these InstantLoader pointers. Or, if you like, Jered could remove them here and I'll add them back in when I need them :) > > http://codereview.chromium.**org/10732002/<http://codereview.chromium.org/107... >
That makes sense. -Scott On Thu, Jul 19, 2012 at 1:47 PM, Dominic Hamon <dominich@chromium.org> wrote: > > On Thu, Jul 19, 2012 at 12:35 PM, <sky@chromium.org> wrote: >> >> On 2012/07/19 18:03:43, dominich wrote: >> >> >> http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... >>> >>> File chrome/browser/instant/instant_loader_delegate.h (right): >> >> >> >> >> http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... >>> >>> chrome/browser/instant/instant_loader_delegate.h:24: InstantLoader* >>> loader, >>> On 2012/07/19 17:55:19, sky wrote: >>> > Passing in the loader was really only necessary when multiple loaders >>> > were >>> used. >>> > Since we don't need that functionality anymore remove the loader >>> > argument >> >> from >>> >>> > all of these. >> >> >>> There will be other loaders in the future. For instance, at some point we >>> will >>> want to have a URL Loader that shows previews of URLs. As such, these are >> >> still >>> >>> important to have. >> >> >> If that's that case, then why are we ripping out the url loading feature >> and >> lots of the other stuff in InstantLoader? For example, waiting for a >> paint, >> listening for 403... > > > The URL loading in Instant was poorly implemented and doesn't take into > account many of the issues encountered when trying to preview URLs that the > user hasn't explicitly navigated to. We have a version coming that is based > on Prerendering that does take those things into account - it is implemented > as a different type of InstantLoader. This keeps the code much cleaner and > allows smooth interplay between search instant and url instant. > > Ie, this CL should be considered as abandoning URL instant. > > A future CL will (if it lands ;)) reintroduce URL instant in a much cleaner > way. To do so, we need these InstantLoader pointers. > > Or, if you like, Jered could remove them here and I'll add them back in when > I need them :) > >> >> >> http://codereview.chromium.org/10732002/ > >
http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.cc (left): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:266: gfx::NativeView view_gaining_focus) { On 2012/07/19 17:55:19, sky wrote: > Why is it safe to nuke all this code? I don't know. I think we now hide instead of destroy, and will commit on mouseup instead; but again I'll defer to Sreeram. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.cc (right): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:50: PREVIEW_NUM_TYPES, On 2012/07/19 17:55:19, sky wrote: > If you're going to have NUM_TYPES, explicitly set CREATED to 0. Done. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:152: if (!tab_contents) { On 2012/07/19 17:55:19, sky wrote: > It doesn't make sense to have a DCHECK then a conditional. If > GetInstantHostTabContents() never returns NULL, then a DCHECK and no if. Done. This ultimately calls chrome::GetActiveTabContents(), which can return NULL if active_index() is not actually valid, but I don't think that can happen... http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:183: // call |Update| again, at which time we'll update last_full_text_. On 2012/07/19 17:55:19, sky wrote: > |last_full_text_| Done. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:270: DeleteLoader(); On 2012/07/19 17:55:19, sky wrote: > Add a comment as to why DeleteLoader is invoked after the DeleteSoon call. Done. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:483: *instant_url = instant_url_ref.SupportsReplacement() ? On 2012/07/19 17:55:19, sky wrote: > If the instant_url_ref doesn't support replacement this should return false. Done. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:29: // |InstantController| maintains a WebContents that is intended to give a On 2012/07/19 17:55:19, sky wrote: > Use |s for members/arguments, () for functions and generally nothing for names. Done. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:114: WARN_UNUSED_RESULT; On 2012/07/19 17:55:19, sky wrote: > Wrap 'InstantCommitType type' so that WARN_UNUSED_RESULT isn't dangling (similar > to how const is treated). Done. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:157: // If |template_url| is a valid TemplateURL for use with Instant, fills in On 2012/07/19 17:55:19, sky wrote: > Description should include kInstantURL. Done. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:204: bool loader_ready_; On 2012/07/19 17:55:19, sky wrote: > This name is confusing. Its not whether the loader is ready, rather the loader > has processed the last request. Maybe loader_processed_last_update_. Done. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:220: std::map<string16, int> blacklisted_ids_; On 2012/07/19 17:55:19, sky wrote: > This assumes that once it sees a keyword the TemplateURL associated with it > never changes. That is wrong. There is nothing stopping the user from > manipulating the set of keywords in various ways during the session. What is the proper id to use? Does this need to map from URLs? http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader.cc (right): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:113: // If this is being called, something is swapping in to our preview_contents_ On 2012/07/19 17:55:19, sky wrote: > |preview_contents_| Done. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:260: SetupPreviewContents(); On 2012/07/19 17:55:19, sky wrote: > These are expensive operations and should be moved to an Init methods. Done. http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:290: const std::string& text) { On 2012/07/19 17:55:19, sky wrote: > This should take string16. If it needs to convert to utf8 for the messages, do > it here. Done. http://codereview.chromium.org/10732002/diff/19001/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10732002/diff/19001/chrome/common/instant_type... chrome/common/instant_types.h:15: // Autocomplete the suggestion after a delay. On 2012/07/19 17:55:19, sky wrote: > Its my understanding we're not using this, can it be removed now? It's still referenced several places in the code, so naively, I don't think so.
http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:220: std::map<string16, int> blacklisted_ids_; On 2012/07/19 22:01:33, Jered wrote: > On 2012/07/19 17:55:19, sky wrote: > > This assumes that once it sees a keyword the TemplateURL associated with it > > never changes. That is wrong. There is nothing stopping the user from > > manipulating the set of keywords in various ways during the session. > > What is the proper id to use? Does this need to map from URLs? The id from the TemplateURL never changes. That said, the user can change the TemplateURL, but not the instant_url (we don't surface ui for that).
http://codereview.chromium.org/10732002/diff/19001/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10732002/diff/19001/chrome/common/instant_type... chrome/common/instant_types.h:15: // Autocomplete the suggestion after a delay. On 2012/07/19 22:01:33, Jered wrote: > On 2012/07/19 17:55:19, sky wrote: > > Its my understanding we're not using this, can it be removed now? > > It's still referenced several places in the code, so naively, I don't think so. I think it can be removed from all of them. But, that can be done later.
http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:220: std::map<string16, int> blacklisted_ids_; On 2012/07/19 23:43:49, sky wrote: > On 2012/07/19 22:01:33, Jered wrote: > > On 2012/07/19 17:55:19, sky wrote: > > > This assumes that once it sees a keyword the TemplateURL associated with it > > > never changes. That is wrong. There is nothing stopping the user from > > > manipulating the set of keywords in various ways during the session. > > > > What is the proper id to use? Does this need to map from URLs? > > The id from the TemplateURL never changes. That said, the user can change the > TemplateURL, but not the instant_url (we don't surface ui for that). How about just using the instant url? http://codereview.chromium.org/10732002/diff/19001/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10732002/diff/19001/chrome/common/instant_type... chrome/common/instant_types.h:15: // Autocomplete the suggestion after a delay. On 2012/07/19 23:44:40, sky wrote: > On 2012/07/19 22:01:33, Jered wrote: > > On 2012/07/19 17:55:19, sky wrote: > > > Its my understanding we're not using this, can it be removed now? > > > > It's still referenced several places in the code, so naively, I don't think > so. > > I think it can be removed from all of them. But, that can be done later. I will wait, then.
On Fri, Jul 20, 2012 at 9:10 AM, <jered@chromium.org> wrote: > > http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... > File chrome/browser/instant/instant_controller.h (right): > > http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... > chrome/browser/instant/instant_controller.h:220: std::map<string16, int> > blacklisted_ids_; > On 2012/07/19 23:43:49, sky wrote: >> >> On 2012/07/19 22:01:33, Jered wrote: >> > On 2012/07/19 17:55:19, sky wrote: >> > > This assumes that once it sees a keyword the TemplateURL > > associated with it >> >> > > never changes. That is wrong. There is nothing stopping the user > > from >> >> > > manipulating the set of keywords in various ways during the > > session. >> >> > >> > What is the proper id to use? Does this need to map from URLs? > > >> The id from the TemplateURL never changes. That said, the user can > > change the >> >> TemplateURL, but not the instant_url (we don't surface ui for that). > > > How about just using the instant url? Yes, I think that is the right thing to do. -Scott > > > http://codereview.chromium.org/10732002/diff/19001/chrome/common/instant_types.h > File chrome/common/instant_types.h (right): > > http://codereview.chromium.org/10732002/diff/19001/chrome/common/instant_type... > chrome/common/instant_types.h:15: // Autocomplete the suggestion after a > delay. > On 2012/07/19 23:44:40, sky wrote: >> >> On 2012/07/19 22:01:33, Jered wrote: >> > On 2012/07/19 17:55:19, sky wrote: >> > > Its my understanding we're not using this, can it be removed now? >> > >> > It's still referenced several places in the code, so naively, I > > don't think >> >> so. > > >> I think it can be removed from all of them. But, that can be done > > later. > > I will wait, then. > > http://codereview.chromium.org/10732002/
http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:220: std::map<string16, int> blacklisted_ids_; On 2012/07/20 16:10:18, Jered wrote: > On 2012/07/19 23:43:49, sky wrote: > > On 2012/07/19 22:01:33, Jered wrote: > > > On 2012/07/19 17:55:19, sky wrote: > > > > This assumes that once it sees a keyword the TemplateURL associated with > it > > > > never changes. That is wrong. There is nothing stopping the user from > > > > manipulating the set of keywords in various ways during the session. > > > > > > What is the proper id to use? Does this need to map from URLs? > > > > The id from the TemplateURL never changes. That said, the user can change the > > TemplateURL, but not the instant_url (we don't surface ui for that). > > How about just using the instant url? Done.
We're still waiting to hear from Sreeram on ripping out the focus related code. -Scott On Fri, Jul 20, 2012 at 2:21 PM, <jered@chromium.org> wrote: > > http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... > File chrome/browser/instant/instant_controller.h (right): > > http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... > chrome/browser/instant/instant_controller.h:220: std::map<string16, int> > blacklisted_ids_; > On 2012/07/20 16:10:18, Jered wrote: >> >> On 2012/07/19 23:43:49, sky wrote: >> > On 2012/07/19 22:01:33, Jered wrote: >> > > On 2012/07/19 17:55:19, sky wrote: >> > > > This assumes that once it sees a keyword the TemplateURL > > associated with >> >> it >> > > > never changes. That is wrong. There is nothing stopping the user > > from >> >> > > > manipulating the set of keywords in various ways during the > > session. >> >> > > >> > > What is the proper id to use? Does this need to map from URLs? >> > >> > The id from the TemplateURL never changes. That said, the user can > > change the >> >> > TemplateURL, but not the instant_url (we don't surface ui for that). > > >> How about just using the instant url? > > > Done. > > http://codereview.chromium.org/10732002/
On Mon, Jul 23, 2012 at 8:27 AM, Scott Violet <sky@chromium.org> wrote: > We're still waiting to hear from Sreeram on ripping out the focus related code. I'm back now. Will respond shortly (once I read through the thread).
http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.cc (left): http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.cc:266: gfx::NativeView view_gaining_focus) { On 2012/07/19 22:01:33, Jered wrote: > On 2012/07/19 17:55:19, sky wrote: > > Why is it safe to nuke all this code? > > I don't know. I think we now hide instead of destroy, and will commit on mouseup > instead; but again I'll defer to Sreeram. Most of the previous code was to destroy the preview when the omnibox loses focus. As Jered mentioned, we now just want to Hide(), and it can be reduced to a much simpler test, which is whether we are in "commit on mouse up" or not. There's one situation where the old code commits where we now don't (and therefore we'll just Hide()), which is the paragraph starting with "Walk up the view hierarchy", which handles the user clicking on a windowed plugin or http auth dialog. Neither of these should occur on a search results page, and I'm fine with hiding the results page if that happens (and any user feedback about it happening would be a cue for the search provider to fix their page).
On Mon, Jul 23, 2012 at 9:55 AM, <sreeram@chromium.org> wrote: > > http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... > File chrome/browser/instant/instant_controller.cc (left): > > http://codereview.chromium.org/10732002/diff/19001/chrome/browser/instant/ins... > chrome/browser/instant/instant_controller.cc:266: gfx::NativeView > view_gaining_focus) { > On 2012/07/19 22:01:33, Jered wrote: >> >> On 2012/07/19 17:55:19, sky wrote: >> > Why is it safe to nuke all this code? > > >> I don't know. I think we now hide instead of destroy, and will commit > > on mouseup >> >> instead; but again I'll defer to Sreeram. > > > Most of the previous code was to destroy the preview when the omnibox > loses focus. As Jered mentioned, we now just want to Hide(), and it can > be reduced to a much simpler test, which is whether we are in "commit on > mouse up" or not. There are situations where the previous code would commit (out side of from the mouse). Are those handled? The primary one is the page getting focus. > > There's one situation where the old code commits where we now don't (and > therefore we'll just Hide()), which is the paragraph starting with "Walk > up the view hierarchy", which handles the user clicking on a windowed > plugin or http auth dialog. Neither of these should occur on a search > results page, and I'm fine with hiding the results page if that happens > (and any user feedback about it happening would be a cue for the search > provider to fix their page). This code is very subtle. Dominic mentioned we may bring back the URL loading, at which point wouldn't this code be needed? -Scott
On Mon, Jul 23, 2012 at 11:26 AM, Scott Violet <sky@chromium.org> wrote: > On Mon, Jul 23, 2012 at 9:55 AM, <sreeram@chromium.org> wrote: >> Most of the previous code was to destroy the preview when the omnibox >> loses focus. As Jered mentioned, we now just want to Hide(), and it can >> be reduced to a much simpler test, which is whether we are in "commit on >> mouse up" or not. > > There are situations where the previous code would commit (out side of > from the mouse). Are those handled? The primary one is the page > getting focus. Outside of a mouse click (and the recently fixed touch-gesture, see http://crrev.com/147456), how else would the preview get focus? <Tab> doesn't do it. I'm trying to think of other cases (perhaps the page itself requesting / grabbing focus?), but am unable to come up with any. >> There's one situation where the old code commits where we now don't (and >> therefore we'll just Hide()), which is the paragraph starting with "Walk >> up the view hierarchy", which handles the user clicking on a windowed >> plugin or http auth dialog. Neither of these should occur on a search >> results page, and I'm fine with hiding the results page if that happens >> (and any user feedback about it happening would be a cue for the search >> provider to fix their page). > > This code is very subtle. Dominic mentioned we may bring back the URL > loading, at which point wouldn't this code be needed? Possible. But even then, we only will need to handle a couple of cases, so I think it's still worth it to rip it all out now, and put back only what's absolutely needed. For instance, depending on what the WebContentsDelegateImpl allows or disallows, some of these scenarios (http-auth, for example) may end up cancelling the preview, so we won't have to worry about committing or hiding due to it.
On Mon, Jul 23, 2012 at 12:33 PM, Sreeram Ramachandran <sreeram@chromium.org> wrote: > On Mon, Jul 23, 2012 at 11:26 AM, Scott Violet <sky@chromium.org> wrote: >> On Mon, Jul 23, 2012 at 9:55 AM, <sreeram@chromium.org> wrote: >>> Most of the previous code was to destroy the preview when the omnibox >>> loses focus. As Jered mentioned, we now just want to Hide(), and it can >>> be reduced to a much simpler test, which is whether we are in "commit on >>> mouse up" or not. >> >> There are situations where the previous code would commit (out side of >> from the mouse). Are those handled? The primary one is the page >> getting focus. > > Outside of a mouse click (and the recently fixed touch-gesture, see > http://crrev.com/147456), how else would the preview get focus? <Tab> > doesn't do it. Why can't tab do it? -Scott I'm trying to think of other cases (perhaps the page > itself requesting / grabbing focus?), but am unable to come up with > any. > >>> There's one situation where the old code commits where we now don't (and >>> therefore we'll just Hide()), which is the paragraph starting with "Walk >>> up the view hierarchy", which handles the user clicking on a windowed >>> plugin or http auth dialog. Neither of these should occur on a search >>> results page, and I'm fine with hiding the results page if that happens >>> (and any user feedback about it happening would be a cue for the search >>> provider to fix their page). >> >> This code is very subtle. Dominic mentioned we may bring back the URL >> loading, at which point wouldn't this code be needed? > > Possible. But even then, we only will need to handle a couple of > cases, so I think it's still worth it to rip it all out now, and put > back only what's absolutely needed. For instance, depending on what > the WebContentsDelegateImpl allows or disallows, some of these > scenarios (http-auth, for example) may end up cancelling the preview, > so we won't have to worry about committing or hiding due to it.
On Mon, Jul 23, 2012 at 2:14 PM, Scott Violet <sky@chromium.org> wrote: > On Mon, Jul 23, 2012 at 12:33 PM, Sreeram Ramachandran > <sreeram@chromium.org> wrote: >> On Mon, Jul 23, 2012 at 11:26 AM, Scott Violet <sky@chromium.org> wrote: >>> On Mon, Jul 23, 2012 at 9:55 AM, <sreeram@chromium.org> wrote: >>>> Most of the previous code was to destroy the preview when the omnibox >>>> loses focus. As Jered mentioned, we now just want to Hide(), and it can >>>> be reduced to a much simpler test, which is whether we are in "commit on >>>> mouse up" or not. >>> >>> There are situations where the previous code would commit (out side of >>> from the mouse). Are those handled? The primary one is the page >>> getting focus. >> >> Outside of a mouse click (and the recently fixed touch-gesture, see >> http://crrev.com/147456), how else would the preview get focus? <Tab> >> doesn't do it. > > Why can't tab do it? <Tab> steps through the omnibox dropdown. When you reach the end of the list, <Tab> brings you back to the top again, instead of sending focus into the content view. It also seems to me that we (Instant) don't _want_ to get focus through any other means (other than mouse click) based on the call to rwhv->SetTakesFocusOnlyOnMouseDown(true) (at least for the Mac platform).
On Mon, Jul 23, 2012 at 2:19 PM, Sreeram Ramachandran <sreeram@chromium.org> wrote: > On Mon, Jul 23, 2012 at 2:14 PM, Scott Violet <sky@chromium.org> wrote: >> On Mon, Jul 23, 2012 at 12:33 PM, Sreeram Ramachandran >> <sreeram@chromium.org> wrote: >>> On Mon, Jul 23, 2012 at 11:26 AM, Scott Violet <sky@chromium.org> wrote: >>>> On Mon, Jul 23, 2012 at 9:55 AM, <sreeram@chromium.org> wrote: >>>>> Most of the previous code was to destroy the preview when the omnibox >>>>> loses focus. As Jered mentioned, we now just want to Hide(), and it can >>>>> be reduced to a much simpler test, which is whether we are in "commit on >>>>> mouse up" or not. >>>> >>>> There are situations where the previous code would commit (out side of >>>> from the mouse). Are those handled? The primary one is the page >>>> getting focus. >>> >>> Outside of a mouse click (and the recently fixed touch-gesture, see >>> http://crrev.com/147456), how else would the preview get focus? <Tab> >>> doesn't do it. >> >> Why can't tab do it? > > <Tab> steps through the omnibox dropdown. When you reach the end of > the list, <Tab> brings you back to the top again, instead of sending > focus into the content view. It also seems to me that we (Instant) > don't _want_ to get focus through any other means (other than mouse > click) based on the call to rwhv->SetTakesFocusOnlyOnMouseDown(true) > (at least for the Mac platform). I haven't traced through the code, does tab if there is only one entry focus the content? -Scott
LGTM I haven't looked super thoroughly; I'll handle the fallout if any. http://codereview.chromium.org/10732002/diff/20029/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader.h (right): http://codereview.chromium.org/10732002/diff/20029/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.h:64: const string16& text) WARN_UNUSED_RESULT; On Thu, Jul 19, 2012 at 10:55 AM, <sky@chromium.org> wrote: > This should take string16. If it needs to convert to utf8 for the > messages, do it here. Why is that? The idea I was going for is that all conversions (between string16 and std::string) happens in InstantController. Everything downstream of that (including InstantLoader, SearchBox, SearchBoxExtension, and the IPCs) all use std::string. This not only seemed cleaner (all conversions in one place), but also seemed to yield the fewest unnecessary conversions. http://codereview.chromium.org/10732002/diff/20029/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader_delegate.h (right): http://codereview.chromium.org/10732002/diff/20029/chrome/browser/instant/ins... chrome/browser/instant/instant_loader_delegate.h:32: virtual void InstantLoaderPreviewLoaded(InstantLoader* loader) = 0; On Thu, Jul 19, 2012 at 11:03 AM, <dominich@chromium.org> wrote: > > On 2012/07/19 17:55:19, sky wrote: >> >> Passing in the loader was really only necessary when multiple loaders were used. >> Since we don't need that functionality anymore remove the loader argument from >> all of these. > > There will be other loaders in the future. For instance, at some point > we will want to have a URL Loader that shows previews of URLs. As such, > these are still important to have. There's one more reason to keep the loader argument: Consider the cases when the InstantController schedules a loader for destruction (instead of deleting it immediately). It's possible that the still-alive-loader may receive an event and call its delegate (InstantController). The controller should ignore such callbacks, which is why InstantController has this pattern: void SomeInstantLoaderDelegateMethodImpl(InstantLoader* loader, ...) { if (loader_ != loader) { return; } ... } This is in spirit similar to how the InstantLoader itself checks page_id when it receives a message from the renderer. http://codereview.chromium.org/10732002/diff/20029/chrome/browser/ui/gtk/tab_... File chrome/browser/ui/gtk/tab_contents_container_gtk.cc (right): http://codereview.chromium.org/10732002/diff/20029/chrome/browser/ui/gtk/tab_... chrome/browser/ui/gtk/tab_contents_container_gtk.cc:113: gtk_container_remove(GTK_CONTAINER(expanded_), preview_widget); Revert the changes to this file (they seem unnecessary). http://codereview.chromium.org/10732002/diff/20029/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): http://codereview.chromium.org/10732002/diff/20029/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.h:516: InstantCompleteBehavior instant_complete_behavior_; We can get rid of this member now. http://codereview.chromium.org/10732002/diff/20029/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10732002/diff/20029/chrome/common/instant_type... chrome/common/instant_types.h:21: INSTANT_COMPLETE_NEVER Add a trailing comma. Makes the diff simpler when adding new types to the list. http://codereview.chromium.org/10732002/diff/20029/content/browser/web_conten... File content/browser/web_contents/web_contents_impl.cc (right): http://codereview.chromium.org/10732002/diff/20029/content/browser/web_conten... content/browser/web_contents/web_contents_impl.cc:928: LOG(WARNING) << "Restoring web contents with no rwhv"; Revert the changes to this file (they seem unnecessary). http://codereview.chromium.org/10732002/diff/25005/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10732002/diff/25005/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:57: SILENT Add a comma at the end (makes the diff simpler when extending the enum).
On Mon, Jul 23, 2012 at 8:35 PM, Scott Violet <sky@chromium.org> wrote: > On Mon, Jul 23, 2012 at 2:19 PM, Sreeram Ramachandran > <sreeram@chromium.org> wrote: >> <Tab> steps through the omnibox dropdown. When you reach the end of >> the list, <Tab> brings you back to the top again, instead of sending >> focus into the content view. It also seems to me that we (Instant) >> don't _want_ to get focus through any other means (other than mouse >> click) based on the call to rwhv->SetTakesFocusOnlyOnMouseDown(true) >> (at least for the Mac platform). > > I haven't traced through the code, does tab if there is only one entry > focus the content? Nope. I had tested that as well. Still no focus transfer. +pkasting Peter: With Instant showing, and keyboard focus in the omnibox, are there any ways you can think of that can transfer focus to the content view, short of the user clicking on the content?
Here's a new patch set. http://codereview.chromium.org/10732002/diff/20029/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader.h (right): http://codereview.chromium.org/10732002/diff/20029/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.h:64: const string16& text) WARN_UNUSED_RESULT; On 2012/07/24 14:08:32, sreeram wrote: > On Thu, Jul 19, 2012 at 10:55 AM, <mailto:sky@chromium.org> wrote: > > This should take string16. If it needs to convert to utf8 for the > > messages, do it here. > > Why is that? The idea I was going for is that all conversions (between string16 > and std::string) happens in InstantController. Everything downstream of that > (including InstantLoader, SearchBox, SearchBoxExtension, and the IPCs) all use > std::string. This not only seemed cleaner (all conversions in one place), but > also seemed to yield the fewest unnecessary conversions. I am fairly neutral, but have gone with Sreeram's suggestion because it seems better to have some convention. http://codereview.chromium.org/10732002/diff/20029/chrome/browser/ui/gtk/tab_... File chrome/browser/ui/gtk/tab_contents_container_gtk.cc (right): http://codereview.chromium.org/10732002/diff/20029/chrome/browser/ui/gtk/tab_... chrome/browser/ui/gtk/tab_contents_container_gtk.cc:113: gtk_container_remove(GTK_CONTAINER(expanded_), preview_widget); On 2012/07/24 14:08:32, sreeram wrote: > Revert the changes to this file (they seem unnecessary). Done. http://codereview.chromium.org/10732002/diff/20029/chrome/browser/ui/omnibox/... File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): http://codereview.chromium.org/10732002/diff/20029/chrome/browser/ui/omnibox/... chrome/browser/ui/omnibox/omnibox_edit_model.h:516: InstantCompleteBehavior instant_complete_behavior_; On 2012/07/24 14:08:32, sreeram wrote: > We can get rid of this member now. It's referenced in instant_browsertest and seems appropriate in context there. I've updated this comment. Let me know if you can think of a better way. http://codereview.chromium.org/10732002/diff/20029/chrome/common/instant_types.h File chrome/common/instant_types.h (right): http://codereview.chromium.org/10732002/diff/20029/chrome/common/instant_type... chrome/common/instant_types.h:21: INSTANT_COMPLETE_NEVER On 2012/07/24 14:08:32, sreeram wrote: > Add a trailing comma. Makes the diff simpler when adding new types to the list. Done. http://codereview.chromium.org/10732002/diff/20029/content/browser/web_conten... File content/browser/web_contents/web_contents_impl.cc (right): http://codereview.chromium.org/10732002/diff/20029/content/browser/web_conten... content/browser/web_contents/web_contents_impl.cc:928: LOG(WARNING) << "Restoring web contents with no rwhv"; On 2012/07/24 14:08:32, sreeram wrote: > Revert the changes to this file (they seem unnecessary). Done. http://codereview.chromium.org/10732002/diff/25005/chrome/browser/instant/ins... File chrome/browser/instant/instant_controller.h (right): http://codereview.chromium.org/10732002/diff/25005/chrome/browser/instant/ins... chrome/browser/instant/instant_controller.h:57: SILENT On 2012/07/24 14:08:33, sreeram wrote: > Add a comma at the end (makes the diff simpler when extending the enum). Done.
LGTM Please also run the pyauto tests (chrome/test/functional/instant.py).
On Tue, Jul 24, 2012 at 7:08 AM, <sreeram@chromium.org> wrote: > LGTM > > I haven't looked super thoroughly; I'll handle the fallout if any. > > > http://codereview.chromium.org/10732002/diff/20029/chrome/browser/instant/ins... > File chrome/browser/instant/instant_loader.h (right): > > http://codereview.chromium.org/10732002/diff/20029/chrome/browser/instant/ins... > chrome/browser/instant/instant_loader.h:64: const string16& text) > WARN_UNUSED_RESULT; > > On Thu, Jul 19, 2012 at 10:55 AM, <sky@chromium.org> wrote: >> >> This should take string16. If it needs to convert to utf8 for the >> messages, do it here. > > > Why is that? The idea I was going for is that all conversions (between > string16 and std::string) happens in InstantController. Everything > downstream of that (including InstantLoader, SearchBox, > SearchBoxExtension, and the IPCs) all use std::string. This not only > seemed cleaner (all conversions in one place), but also seemed to yield > the fewest unnecessary conversions. You've converted a number of places that were previously string16 to string. I don't understand the motivation for that. > http://codereview.chromium.org/10732002/diff/20029/chrome/browser/instant/ins... > File chrome/browser/instant/instant_loader_delegate.h (right): > > http://codereview.chromium.org/10732002/diff/20029/chrome/browser/instant/ins... > chrome/browser/instant/instant_loader_delegate.h:32: virtual void > InstantLoaderPreviewLoaded(InstantLoader* loader) = 0; > > On Thu, Jul 19, 2012 at 11:03 AM, <dominich@chromium.org> wrote: > >> On 2012/07/19 17:55:19, sky wrote: > > >>> Passing in the loader was really only necessary when multiple loaders > > were used. >>> >>> Since we don't need that functionality anymore remove the loader > > argument from >>> >>> all of these. > > >> There will be other loaders in the future. For instance, at some point >> we will want to have a URL Loader that shows previews of URLs. As > > such, >> >> these are still important to have. > > > There's one more reason to keep the loader argument: Consider the cases > when the InstantController schedules a loader for destruction (instead > of deleting it immediately). It's possible that the still-alive-loader > may receive an event and call its delegate (InstantController). The > controller should ignore such callbacks, which is why InstantController > has this pattern: Good point. > void SomeInstantLoaderDelegateMethodImpl(InstantLoader* loader, ...) { > if (loader_ != loader) { > return; > } > ... > } > > This is in spirit similar to how the InstantLoader itself checks page_id > when it receives a message from the renderer. > > http://codereview.chromium.org/10732002/diff/20029/chrome/browser/ui/gtk/tab_... > File chrome/browser/ui/gtk/tab_contents_container_gtk.cc (right): > > http://codereview.chromium.org/10732002/diff/20029/chrome/browser/ui/gtk/tab_... > chrome/browser/ui/gtk/tab_contents_container_gtk.cc:113: > gtk_container_remove(GTK_CONTAINER(expanded_), preview_widget); > Revert the changes to this file (they seem unnecessary). > > http://codereview.chromium.org/10732002/diff/20029/chrome/browser/ui/omnibox/... > File chrome/browser/ui/omnibox/omnibox_edit_model.h (right): > > http://codereview.chromium.org/10732002/diff/20029/chrome/browser/ui/omnibox/... > chrome/browser/ui/omnibox/omnibox_edit_model.h:516: > InstantCompleteBehavior instant_complete_behavior_; > We can get rid of this member now. > > http://codereview.chromium.org/10732002/diff/20029/chrome/common/instant_types.h > File chrome/common/instant_types.h (right): > > http://codereview.chromium.org/10732002/diff/20029/chrome/common/instant_type... > chrome/common/instant_types.h:21: INSTANT_COMPLETE_NEVER > Add a trailing comma. Makes the diff simpler when adding new types to > the list. > > http://codereview.chromium.org/10732002/diff/20029/content/browser/web_conten... > File content/browser/web_contents/web_contents_impl.cc (right): > > http://codereview.chromium.org/10732002/diff/20029/content/browser/web_conten... > content/browser/web_contents/web_contents_impl.cc:928: LOG(WARNING) << > "Restoring web contents with no rwhv"; > Revert the changes to this file (they seem unnecessary). > > http://codereview.chromium.org/10732002/diff/25005/chrome/browser/instant/ins... > File chrome/browser/instant/instant_controller.h (right): > > http://codereview.chromium.org/10732002/diff/25005/chrome/browser/instant/ins... > chrome/browser/instant/instant_controller.h:57: SILENT > Add a comma at the end (makes the diff simpler when extending the enum). > > http://codereview.chromium.org/10732002/
You're also going to need to sync up. Sadrul renamed a couple of things to support gestures.
On Tue, Jul 24, 2012 at 9:36 AM, Scott Violet <sky@chromium.org> wrote: > You've converted a number of places that were previously string16 to > string. I don't understand the motivation for that. All the std::strings I see are those that are passed on to the renderer. I think this is necessary, since JS deals with only UTF-8. For example, previously, SearchBoxExtensionWrapper::GetValue() would convert from string16 each time the user text was accessed. Now, we convert once, in InstantController, and send it over to the renderer. We don't pay the cost of conversion each time the JS accesses the value. Anything that's not sent over to the renderer is not converted, for example, last_full_text_ or last_user_text_ in InstantController. Is there something I am missing?
On Tue, Jul 24, 2012 at 9:43 AM, Sreeram Ramachandran <sreeram@chromium.org> wrote: > On Tue, Jul 24, 2012 at 9:36 AM, Scott Violet <sky@chromium.org> wrote: >> You've converted a number of places that were previously string16 to >> string. I don't understand the motivation for that. > > All the std::strings I see are those that are passed on to the > renderer. I think this is necessary, since JS deals with only UTF-8. > For example, previously, SearchBoxExtensionWrapper::GetValue() would > convert from string16 each time the user text was accessed. Now, we > convert once, in InstantController, and send it over to the renderer. > We don't pay the cost of conversion each time the JS accesses the > value. > > Anything that's not sent over to the renderer is not converted, for > example, last_full_text_ or last_user_text_ in InstantController. Is > there something I am missing? That makes sense, thanks for the clarification. -Scott
On Tue, Jul 24, 2012 at 9:52 AM, Scott Violet <sky@chromium.org> wrote: > On Tue, Jul 24, 2012 at 9:43 AM, Sreeram Ramachandran > <sreeram@chromium.org> wrote: >> On Tue, Jul 24, 2012 at 9:36 AM, Scott Violet <sky@chromium.org> wrote: >>> You've converted a number of places that were previously string16 to >>> string. I don't understand the motivation for that. >> >> All the std::strings I see are those that are passed on to the >> renderer. I think this is necessary, since JS deals with only UTF-8. >> For example, previously, SearchBoxExtensionWrapper::GetValue() would >> convert from string16 each time the user text was accessed. Now, we >> convert once, in InstantController, and send it over to the renderer. >> We don't pay the cost of conversion each time the JS accesses the >> value. >> >> Anything that's not sent over to the renderer is not converted, for >> example, last_full_text_ or last_user_text_ in InstantController. Is >> there something I am missing? > > That makes sense, thanks for the clarification. Damn! A little knowledge is a dangerous thing. After digging into v8 and some Google searches, it turns out that everything I said is the opposite: Strings in JS are all UTF-16! So, in fact, supplying a UTF-8 string is the one that causes conversions into UTF-16 each time. Gah. Jered, apologies for this huge blunder. Could you turn everything into string16s everywhere?
On Tue, Jul 24, 2012 at 7:12 AM, Sreeram Ramachandran <sreeram@chromium.org>wrote: > Peter: With Instant showing, and keyboard focus in the omnibox, are > there any ways you can think of that can transfer focus to the content > view, short of the user clicking on the content? > F6 should work, I think. PK
On 2012/07/24 17:56:44, sreeram wrote: > On Tue, Jul 24, 2012 at 9:52 AM, Scott Violet <mailto:sky@chromium.org> wrote: > > On Tue, Jul 24, 2012 at 9:43 AM, Sreeram Ramachandran > > <mailto:sreeram@chromium.org> wrote: > >> On Tue, Jul 24, 2012 at 9:36 AM, Scott Violet <mailto:sky@chromium.org> wrote: > >>> You've converted a number of places that were previously string16 to > >>> string. I don't understand the motivation for that. > >> > >> All the std::strings I see are those that are passed on to the > >> renderer. I think this is necessary, since JS deals with only UTF-8. > >> For example, previously, SearchBoxExtensionWrapper::GetValue() would > >> convert from string16 each time the user text was accessed. Now, we > >> convert once, in InstantController, and send it over to the renderer. > >> We don't pay the cost of conversion each time the JS accesses the > >> value. > >> > >> Anything that's not sent over to the renderer is not converted, for > >> example, last_full_text_ or last_user_text_ in InstantController. Is > >> there something I am missing? > > > > That makes sense, thanks for the clarification. > > Damn! A little knowledge is a dangerous thing. After digging into v8 > and some Google searches, it turns out that everything I said is the > opposite: Strings in JS are all UTF-16! So, in fact, supplying a UTF-8 > string is the one that causes conversions into UTF-16 each time. Gah. > > Jered, apologies for this huge blunder. Could you turn everything into > string16s everywhere? Done. I am not able to run pyauto tests, something in my environment, I will figure it out. Also the browser test isn't running anymore on linux after my merge. Something weird, I think the automation system changed. Would appreciate any help in debugging it.
On Tue, Jul 24, 2012 at 11:58 AM, Peter Kasting <pkasting@chromium.org> wrote: > On Tue, Jul 24, 2012 at 7:12 AM, Sreeram Ramachandran <sreeram@chromium.org> > wrote: >> >> Peter: With Instant showing, and keyboard focus in the omnibox, are >> there any ways you can think of that can transfer focus to the content >> view, short of the user clicking on the content? > > F6 should work, I think. Thanks! With today's Instant, F6 destroys the preview, and transfers focus to the underlying page (instead of committing the Instant preview and/or transferring focus to it). Jered's CL won't change that behaviour.
On Tue, Jul 24, 2012 at 12:39 PM, Sreeram Ramachandran <sreeram@chromium.org> wrote: > On Tue, Jul 24, 2012 at 11:58 AM, Peter Kasting <pkasting@chromium.org> wrote: >> On Tue, Jul 24, 2012 at 7:12 AM, Sreeram Ramachandran <sreeram@chromium.org> >> wrote: >>> >>> Peter: With Instant showing, and keyboard focus in the omnibox, are >>> there any ways you can think of that can transfer focus to the content >>> view, short of the user clicking on the content? >> >> F6 should work, I think. > > Thanks! With today's Instant, F6 destroys the preview, and transfers > focus to the underlying page (instead of committing the Instant > preview and/or transferring focus to it). Jered's CL won't change that > behaviour. That sounds bad. I wonder what broke that. -Scott
Ok, patchset 13 builds on all platforms. One browser test fails on linux (page visibility). The pyauto tests pass on linux. I am running pyauto_functional_tests on trybots. Please check over patchset 13 and let me know if something looks wrong. On 2012/07/24 21:31:06, sky wrote: > On Tue, Jul 24, 2012 at 12:39 PM, Sreeram Ramachandran > <mailto:sreeram@chromium.org> wrote: > > On Tue, Jul 24, 2012 at 11:58 AM, Peter Kasting <mailto:pkasting@chromium.org> > wrote: > >> On Tue, Jul 24, 2012 at 7:12 AM, Sreeram Ramachandran <mailto:sreeram@chromium.org> > >> wrote: > >>> > >>> Peter: With Instant showing, and keyboard focus in the omnibox, are > >>> there any ways you can think of that can transfer focus to the content > >>> view, short of the user clicking on the content? > >> > >> F6 should work, I think. > > > > Thanks! With today's Instant, F6 destroys the preview, and transfers > > focus to the underlying page (instead of committing the Instant > > preview and/or transferring focus to it). Jered's CL won't change that > > behaviour. > > That sounds bad. I wonder what broke that. > > -Scott
lgtm
http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader.cc (left): http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:339: if (add_page_vector_.empty()) Where is this logic handled now?
http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... File chrome/browser/instant/instant_loader.cc (left): http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... chrome/browser/instant/instant_loader.cc:339: if (add_page_vector_.empty()) On 2012/07/25 16:26:15, sky wrote: > Where is this logic handled now? It's not being handled. I'll be fixing this (adding to history) as one of the first tasks after this CL lands. I've got it mostly figured out now, so should happen asap.
On Wed, Jul 25, 2012 at 9:38 AM, <sreeram@chromium.org> wrote: > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > File chrome/browser/instant/instant_loader.cc (left): > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > chrome/browser/instant/instant_loader.cc:339: if > (add_page_vector_.empty()) > On 2012/07/25 16:26:15, sky wrote: >> >> Where is this logic handled now? > > > It's not being handled. I'll be fixing this (adding to history) as one > of the first tasks after this CL lands. I've got it mostly figured out > now, so should happen asap. Are you sure it can completed by the branch point? -Scott
We shouldn't land a rewrite that is broken in a particular way. Please incorporate into this patchset. -Scott On 2012/07/25 16:50:58, sky wrote: > On Wed, Jul 25, 2012 at 9:38 AM, <mailto:sreeram@chromium.org> wrote: > > > > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > > File chrome/browser/instant/instant_loader.cc (left): > > > > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > > chrome/browser/instant/instant_loader.cc:339: if > > (add_page_vector_.empty()) > > On 2012/07/25 16:26:15, sky wrote: > >> > >> Where is this logic handled now? > > > > > > It's not being handled. I'll be fixing this (adding to history) as one > > of the first tasks after this CL lands. I've got it mostly figured out > > now, so should happen asap. > > Are you sure it can completed by the branch point? > > -Scott
On 2012/07/24 21:31:06, sky wrote: > On Tue, Jul 24, 2012 at 12:39 PM, Sreeram Ramachandran > <mailto:sreeram@chromium.org> wrote: > > On Tue, Jul 24, 2012 at 11:58 AM, Peter Kasting <mailto:pkasting@chromium.org> > wrote: > >> On Tue, Jul 24, 2012 at 7:12 AM, Sreeram Ramachandran <mailto:sreeram@chromium.org> > >> wrote: > >>> > >>> Peter: With Instant showing, and keyboard focus in the omnibox, are > >>> there any ways you can think of that can transfer focus to the content > >>> view, short of the user clicking on the content? > >> > >> F6 should work, I think. > > > > Thanks! With today's Instant, F6 destroys the preview, and transfers > > focus to the underlying page (instead of committing the Instant > > preview and/or transferring focus to it). Jered's CL won't change that > > behaviour. > > That sounds bad. I wonder what broke that. > > -Scott F6 should commit the preview. That it doesn't now is a bug. It seems like some logic related to this is being removed assuming we won't add it back. In particular the code that potentially commits on a focus lost as well as sending verbatim down to the page. I think we should leave this code in so that we can easily get F6 working again. Or maybe I'm missing something? -Scott
On Wed, Jul 25, 2012 at 9:53 AM, <sky@chromium.org> wrote: > We shouldn't land a rewrite that is broken in a particular way. Please > incorporate into this patchset. Will do.
On Wed, Jul 25, 2012 at 10:12 AM, <sky@chromium.org> wrote: > F6 should commit the preview. That it doesn't now is a bug. It seems like some > logic related to this is being removed assuming we won't add it back. In > particular the code that potentially commits on a focus lost as well as sending > verbatim down to the page. I think we should leave this code in so that we can > easily get F6 working again. Or maybe I'm missing something? Fair enough. We'll leave the code in. Could you file a separate bug about F6 and assign it to me? I'll tackle that later.
Couple more nits and questions. http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... File chrome/browser/instant/instant_browsertest.cc (right): http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... chrome/browser/instant/instant_browsertest.cc:54: search_term_(ASCIIToUTF16("def")) { Either put search_term_ on the previous line, or wrap the ':' and fix the indenting. http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... chrome/browser/instant/instant_browsertest.cc:136: string16 last_full_text() const { const string16& http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... chrome/browser/instant/instant_browsertest.cc:823: // in Chrome too. This comment doesn't make sense to me, but I'm assuming it does to dominch. http://codereview.chromium.org/10732002/diff/19039/chrome/browser/ui/browser_... File chrome/browser/ui/browser_instant_controller.cc (left): http://codereview.chromium.org/10732002/diff/19039/chrome/browser/ui/browser_... chrome/browser/ui/browser_instant_controller.cc:93: chrome::GetActiveWebContents(browser_)->WasHidden(); Does this mean both the active and instant preview are visible? http://codereview.chromium.org/10732002/diff/19039/chrome/browser/ui/browser_... File chrome/browser/ui/browser_instant_controller.cc (right): http://codereview.chromium.org/10732002/diff/19039/chrome/browser/ui/browser_... chrome/browser/ui/browser_instant_controller.cc:66: content::PAGE_TRANSITION_GENERATED, How do we know the transition is always GENERATED? Even it is always GENERATED a method to get it on InstantController makes more sense, even it always returns GENERATED.
uploaded patchset 14 to fix a few nits. On 2012/07/25 17:29:56, sky wrote: > Couple more nits and questions. > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > File chrome/browser/instant/instant_browsertest.cc (right): > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > chrome/browser/instant/instant_browsertest.cc:54: > search_term_(ASCIIToUTF16("def")) { > Either put search_term_ on the previous line, or wrap the ':' and fix the > indenting. > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > chrome/browser/instant/instant_browsertest.cc:136: string16 last_full_text() > const { > const string16& > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > chrome/browser/instant/instant_browsertest.cc:823: // in Chrome too. > This comment doesn't make sense to me, but I'm assuming it does to dominch. > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/ui/browser_... > File chrome/browser/ui/browser_instant_controller.cc (left): > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/ui/browser_... > chrome/browser/ui/browser_instant_controller.cc:93: > chrome::GetActiveWebContents(browser_)->WasHidden(); > Does this mean both the active and instant preview are visible? > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/ui/browser_... > File chrome/browser/ui/browser_instant_controller.cc (right): > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/ui/browser_... > chrome/browser/ui/browser_instant_controller.cc:66: > content::PAGE_TRANSITION_GENERATED, > How do we know the transition is always GENERATED? Even it is always GENERATED a > method to get it on InstantController makes more sense, even it always returns > GENERATED.
http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... File chrome/browser/instant/instant_browsertest.cc (right): http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... chrome/browser/instant/instant_browsertest.cc:54: search_term_(ASCIIToUTF16("def")) { On 2012/07/25 17:29:56, sky wrote: > Either put search_term_ on the previous line, or wrap the ':' and fix the > indenting. Done. http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... chrome/browser/instant/instant_browsertest.cc:136: string16 last_full_text() const { On 2012/07/25 17:29:56, sky wrote: > const string16& Done. http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... chrome/browser/instant/instant_browsertest.cc:823: // in Chrome too. On 2012/07/25 17:29:56, sky wrote: > This comment doesn't make sense to me, but I'm assuming it does to dominch. I've deleted the comment. Sreeram will just fix this in his patch.
This has been folded into 10836031. Deleting this issue. On 2012/07/25 20:22:04, jered1 wrote: > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > File chrome/browser/instant/instant_browsertest.cc (right): > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > chrome/browser/instant/instant_browsertest.cc:54: > search_term_(ASCIIToUTF16("def")) { > On 2012/07/25 17:29:56, sky wrote: > > Either put search_term_ on the previous line, or wrap the ':' and fix the > > indenting. > > Done. > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > chrome/browser/instant/instant_browsertest.cc:136: string16 last_full_text() > const { > On 2012/07/25 17:29:56, sky wrote: > > const string16& > > Done. > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > chrome/browser/instant/instant_browsertest.cc:823: // in Chrome too. > On 2012/07/25 17:29:56, sky wrote: > > This comment doesn't make sense to me, but I'm assuming it does to dominch. > > I've deleted the comment. Sreeram will just fix this in his patch.
(Actually, just closed, since the comments could still be useful.) On 2012/07/31 18:48:40, Jered wrote: > This has been folded into 10836031. Deleting this issue. > > On 2012/07/25 20:22:04, jered1 wrote: > > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > > File chrome/browser/instant/instant_browsertest.cc (right): > > > > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > > chrome/browser/instant/instant_browsertest.cc:54: > > search_term_(ASCIIToUTF16("def")) { > > On 2012/07/25 17:29:56, sky wrote: > > > Either put search_term_ on the previous line, or wrap the ':' and fix the > > > indenting. > > > > Done. > > > > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > > chrome/browser/instant/instant_browsertest.cc:136: string16 last_full_text() > > const { > > On 2012/07/25 17:29:56, sky wrote: > > > const string16& > > > > Done. > > > > > http://codereview.chromium.org/10732002/diff/19039/chrome/browser/instant/ins... > > chrome/browser/instant/instant_browsertest.cc:823: // in Chrome too. > > On 2012/07/25 17:29:56, sky wrote: > > > This comment doesn't make sense to me, but I'm assuming it does to dominch. > > > > I've deleted the comment. Sreeram will just fix this in his patch. |
