3 years, 7 months ago
(2017-05-25 03:12:07 UTC)
#6
Dry run: This issue passed the CQ dry run.
xiyuan
https://codereview.chromium.org/2905523004/diff/1/chrome/browser/ui/app_list/search_answer_web_contents_delegate.cc File chrome/browser/ui/app_list/search_answer_web_contents_delegate.cc (right): https://codereview.chromium.org/2905523004/diff/1/chrome/browser/ui/app_list/search_answer_web_contents_delegate.cc#newcode171 chrome/browser/ui/app_list/search_answer_web_contents_delegate.cc:171: render_view_host_view->SetBackgroundColor(SK_ColorTRANSPARENT); There might be a few edge cases. We ...
3 years, 7 months ago
(2017-05-25 16:42:53 UTC)
#7
lgtm https://codereview.chromium.org/2905523004/diff/1/ui/app_list/views/search_result_answer_card_view.cc File ui/app_list/views/search_result_answer_card_view.cc (right): https://codereview.chromium.org/2905523004/diff/1/ui/app_list/views/search_result_answer_card_view.cc#newcode88 ui/app_list/views/search_result_answer_card_view.cc:88: model->AddObserver(this); On 2017/05/25 20:15:16, vadimt wrote: > We ...
3 years, 7 months ago
(2017-05-25 20:52:04 UTC)
#11
lgtm
https://codereview.chromium.org/2905523004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_answer_card_view.cc (right):
https://codereview.chromium.org/2905523004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_answer_card_view.cc:88:
model->AddObserver(this);
On 2017/05/25 20:15:16, vadimt wrote:
> We do this for OnSearchAnswerAvailableChanged() callback to know that
knowledge
> card should be shown/hidden.
>
> As for the "results", there are no results proper, there is just a web view
> always sitting here, and SearchAnswerWebContentsDelegate() occasionally
> proclaiming that it has/hasn't meaningful contents.
>
> So we use SearchResultContainerView as a convenient UI element, but deliver
> results in a custom way.
>
> I considered converting everything to an orthodox result view, with creating a
> search provider that returns a web view passed as a result, but didn't find
big
> value in it, except for some additional engineering work. But it's a question
of
> taste, I guess.
You are right. |model_| is not a SearchResults. The existing code is good.
https://codereview.chromium.org/2905523004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_answer_card_view.h (right):
https://codereview.chromium.org/2905523004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_answer_card_view.h:21: class APP_LIST_EXPORT
SearchResultAnswerCardView
On 2017/05/25 20:15:16, vadimt wrote:
> There are 2 sibling classes:
> SearchResultListView
> SearchResultTileItemListView
> So the naming rule looks like:
> SearchResultXView.
> Since X is AnswerCard (essentially), I came with this name.
>
> (The third sibling, though, doesn't play by rules:
> StartPageView::StartPageTilesContainer, but I prefer following the majority.)
The name of the two siblings are clear to see that they are containers (both
names have "List" in it). They start with SearchResult because they carry views
for app_list::SearchResult. So SearchResultListView is a list of views
representing SearchResult and SearchResultTileItemListView is a list of tile
view of SearchResult. It is clear to tell what they are from their names.
But for this class, it is hard to tell from its name that it is a container for
the actual answer card view. And this class does not have direct relationship
with SearchResult other than it is derived from SearchResultContainerView.
Anyway, it is a nit and I can live with the name.
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
3 years, 7 months ago
(2017-05-25 22:10:26 UTC)
#12
https://codereview.chromium.org/2905523004/diff/1/ui/app_list/views/search_result_answer_card_view.cc File ui/app_list/views/search_result_answer_card_view.cc (right): https://codereview.chromium.org/2905523004/diff/1/ui/app_list/views/search_result_answer_card_view.cc#newcode112 ui/app_list/views/search_result_answer_card_view.cc:112: SetLayoutManager(have_result ? new views::FillLayout : nullptr); On 2017/05/25 22:59:04, ...
3 years, 7 months ago
(2017-05-25 23:01:21 UTC)
#15
https://codereview.chromium.org/2905523004/diff/1/ui/app_list/views/search_re...
File ui/app_list/views/search_result_answer_card_view.cc (right):
https://codereview.chromium.org/2905523004/diff/1/ui/app_list/views/search_re...
ui/app_list/views/search_result_answer_card_view.cc:112:
SetLayoutManager(have_result ? new views::FillLayout : nullptr);
On 2017/05/25 22:59:04, vadimt wrote:
> Actually, there is a bug/feature of FillLayout: it doesn't look at the child's
> visibility. So while the child is not ready to be shown, we happily show an
> empty rectangle that has a child size.
> Restoring the original code.
In this case, since we have only one child, I would prefer to implement
GetPreferredSize and Layout instead of adding/removing FillLayout.
I'm not an owner of content/public/browser. sky->jam
3 years, 7 months ago
(2017-05-25 23:56:08 UTC)
#19
I'm not an owner of content/public/browser.
sky->jam
jam
On 2017/05/25 23:01:34, vadimt wrote: > sky@ please review content/public/browser/* > > This is moving ...
3 years, 7 months ago
(2017-05-26 14:45:12 UTC)
#20
On 2017/05/25 23:01:34, vadimt wrote:
> sky@ please review content/public/browser/*
>
> This is moving some useful files from cros to a more common place.
content/public is only meant to serve as an API to content/, it's not meant to
be a place where helpers are put (see
https://www.chromium.org/developers/content-module/content-api).
vadimt
xiyuan@, given jam@'s comment, any suggestions wrt where to to the background-setting helper for web ...
3 years, 7 months ago
(2017-05-26 20:25:03 UTC)
#21
On 2017/05/26 20:25:03, vadimt wrote: > xiyuan@, given jam@'s comment, any suggestions wrt where to ...
3 years, 7 months ago
(2017-05-26 20:41:55 UTC)
#22
On 2017/05/26 20:25:03, vadimt wrote:
> xiyuan@, given jam@'s comment, any suggestions wrt where to to the
> background-setting helper for web contents?
How about under ui/views/controls/webview ? Since both the login screen and the
answer card UI is on top of views::WebView and needs this, it might make sense
to put it there.
jam
On 2017/05/26 20:41:55, xiyuan wrote: > On 2017/05/26 20:25:03, vadimt wrote: > > xiyuan@, given ...
3 years, 7 months ago
(2017-05-26 22:44:55 UTC)
#23
On 2017/05/26 20:41:55, xiyuan wrote:
> On 2017/05/26 20:25:03, vadimt wrote:
> > xiyuan@, given jam@'s comment, any suggestions wrt where to to the
> > background-setting helper for web contents?
>
> How about under ui/views/controls/webview ? Since both the login screen and
the
> answer card UI is on top of views::WebView and needs this, it might make sense
> to put it there.
sgtm. then it doesnt need to be a separate file, but can just be a method on
that class since it's already a WebContentsObserver?
vadimt
Hmmm, if we want to use the fact the WebView inherits from WebContentsObserver, then we ...
3 years, 7 months ago
(2017-05-26 23:58:43 UTC)
#24
Hmmm, if we want to use the fact the WebView inherits from WebContentsObserver,
then we also have to inherit it from WebContentsUserData, and
WebContentsUserData's comments say:
// A base class for classes attached to, and scoped to, the lifetime of a
// WebContents.
And WebView isn't scoped to WebContents.
This doesn't look quite like something belonging to views:: imho.
Thoughts?
xiyuan
On 2017/05/26 23:58:43, vadimt wrote: > Hmmm, if we want to use the fact the ...
3 years, 6 months ago
(2017-05-30 15:51:51 UTC)
#25
On 2017/05/26 23:58:43, vadimt wrote:
> Hmmm, if we want to use the fact the WebView inherits from
WebContentsObserver,
> then we also have to inherit it from WebContentsUserData, and
> WebContentsUserData's comments say:
> // A base class for classes attached to, and scoped to, the lifetime of a
> // WebContents.
>
> And WebView isn't scoped to WebContents.
>
> This doesn't look quite like something belonging to views:: imho.
>
> Thoughts?
To me, WebContentsSetBackgroundColor class looks like a companion class of
views::WebView since we only need to set bg of a WebContents when it is part of
a WebView. So I think it makes sense to move the code close to views::WebView.
I agree it does not look quite right to combine the code with WebView since
WebView is not bound to a specific WebContents. So I'd say just move the class
there and use the existing/same semantics to use it.
vadimt
Done.
3 years, 6 months ago
(2017-05-30 18:02:50 UTC)
#26
sky@, web_contents_set_background_color ended up in ui/views/controls/webview/, please review (it's just moving).
3 years, 6 months ago
(2017-05-30 18:04:42 UTC)
#28
sky@, web_contents_set_background_color ended up in ui/views/controls/webview/,
please review (it's just moving).
sky
On 2017/05/30 18:04:42, vadimt wrote: > sky@, web_contents_set_background_color ended up in ui/views/controls/webview/, > please review ...
3 years, 6 months ago
(2017-05-30 21:35:20 UTC)
#29
On 2017/05/30 18:04:42, vadimt wrote:
> sky@, web_contents_set_background_color ended up in
ui/views/controls/webview/,
> please review (it's just moving).
Given views doesn't need this code at all I'm hesitant to dump it in views.
Chrome uses app_list, can this code live in app_list and the chrome code gets it
from there?
xiyuan
On 2017/05/30 21:35:20, sky wrote: > On 2017/05/30 18:04:42, vadimt wrote: > > sky@, web_contents_set_background_color ...
3 years, 6 months ago
(2017-05-30 22:00:21 UTC)
#30
On 2017/05/30 21:35:20, sky wrote:
> On 2017/05/30 18:04:42, vadimt wrote:
> > sky@, web_contents_set_background_color ended up in
> ui/views/controls/webview/,
> > please review (it's just moving).
>
> Given views doesn't need this code at all I'm hesitant to dump it in views.
> Chrome uses app_list, can this code live in app_list and the chrome code gets
it
> from there?
The other part that uses this code is WebUILoginView that is part of the login
screen. Making that depends on app list code feels strange.
We used to have only one case (WebUILoginView) so the code is put under
c/b/chromeos/login/ui. Now we have a 2nd case in app list and that is why we
need a new home for the code. I would argue that it makes sense to put the code
close to views::WebView because users of views::WebView might need it. We now
have two case and there might be more in the future.
sky
Ok, fair enough, LGTM
3 years, 6 months ago
(2017-05-30 23:16:07 UTC)
#31
Ok, fair enough, LGTM
vadimt
The CQ bit was checked by vadimt@chromium.org
3 years, 6 months ago
(2017-05-30 23:34:45 UTC)
#32
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/221960) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, ...
3 years, 6 months ago
(2017-05-30 23:39:19 UTC)
#37
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496196365800740, "parent_rev": "be1de5ecae843ecb1fe73577a5f6fb4e51c3f529", "commit_rev": "2420a92712ee82be175bea6750774bec8006c5ce"}
3 years, 6 months ago
(2017-05-31 03:48:31 UTC)
#41
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1496196365800740,
"parent_rev": "be1de5ecae843ecb1fe73577a5f6fb4e51c3f529", "commit_rev":
"2420a92712ee82be175bea6750774bec8006c5ce"}
commit-bot: I haz the power
Description was changed from ========== Making answer card to behave like other results. Now it ...
3 years, 6 months ago
(2017-05-31 03:48:40 UTC)
#42
Message was sent while issue was closed.
Description was changed from
==========
Making answer card to behave like other results.
Now it lives in a search result container, which can be selected,
highlighted, navigated with keyboard etc.
Opening the result (i.e. what happens upon clicking on it) is not yet
implemented.
Unit test for the new result container isn't yet implemented since both
result opening and accessibility behavior are not yet implemented.
Bug=712331
==========
to
==========
Making answer card to behave like other results.
Now it lives in a search result container, which can be selected,
highlighted, navigated with keyboard etc.
Opening the result (i.e. what happens upon clicking on it) is not yet
implemented.
Unit test for the new result container isn't yet implemented since both
result opening and accessibility behavior are not yet implemented.
Bug=712331
Review-Url: https://codereview.chromium.org/2905523004
Cr-Commit-Position: refs/heads/master@{#475782}
Committed:
https://chromium.googlesource.com/chromium/src/+/2420a92712ee82be175bea675077...
==========
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/2420a92712ee82be175bea6750774bec8006c5ce
3 years, 6 months ago
(2017-05-31 03:48:41 UTC)
#43
Description was changed from ========== Making answer card to behave like other results. Now it ...
3 years, 6 months ago
(2017-05-31 19:05:52 UTC)
#47
Message was sent while issue was closed.
Description was changed from
==========
Making answer card to behave like other results.
Now it lives in a search result container, which can be selected,
highlighted, navigated with keyboard etc.
Opening the result (i.e. what happens upon clicking on it) is not yet
implemented.
Unit test for the new result container isn't yet implemented since both
result opening and accessibility behavior are not yet implemented.
Bug=712331
Review-Url: https://codereview.chromium.org/2905523004
Cr-Commit-Position: refs/heads/master@{#475782}
Committed:
https://chromium.googlesource.com/chromium/src/+/2420a92712ee82be175bea675077...
==========
to
==========
Making answer card to behave like other results.
Now it lives in a search result container, which can be selected,
highlighted, navigated with keyboard etc.
Opening the result (i.e. what happens upon clicking on it) is not yet
implemented.
Unit test for the new result container isn't yet implemented since both
result opening and accessibility behavior are not yet implemented.
Bug=712331
Review-Url: https://codereview.chromium.org/2905523004
Cr-Commit-Position: refs/heads/master@{#475782}
Committed:
https://chromium.googlesource.com/chromium/src/+/2420a92712ee82be175bea675077...
==========
vadimt
The CQ bit was checked by vadimt@chromium.org
3 years, 6 months ago
(2017-05-31 19:06:00 UTC)
#48
Issue 2905523004: Making answer card to behave like other results.
(Closed)
Created 3 years, 7 months ago by vadimt
Modified 3 years, 6 months ago
Reviewers: xiyuan, jam, sky
Base URL:
Comments: 22
Chromium Code Reviews
Issue
2905523004:
Making answer card to behave like other results. (Closed)
