Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412423007:
Modify Physical Web Nearby URLs list to resemble search results (Closed)
|
Created:
5 years, 2 months ago by mattreynolds Modified:
5 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionModify Physical Web Nearby URLs list to resemble search results
Add layouts from the Android Physical Web app that display URL items
with blue page titles, green URLs, gray description text, and favicon.
Also add NearbyUrlsAdapter for displaying PwsResults with these layouts.
BUG=529962
Committed: https://crrev.com/b2f96a5eefb9ca66435bba93495b3cddcdcaa80d
Cr-Commit-Position: refs/heads/master@{#356160}
Patch Set 1 #
Total comments: 4
Patch Set 2 : wip (taking changes from dvh,agrieve) #
Total comments: 4
Patch Set 3 : wip (taking changes from mariakhomenko) #
Total comments: 11
Patch Set 4 : wip (taking changes from aurimas) #
Total comments: 3
Patch Set 5 : wip (taking changes from aurimas) #
Total comments: 1
Patch Set 6 : wip (fix maxLines->singleLine) #
Messages
Total messages: 46 (17 generated)
Description was changed from ========== Modify Physical Web Nearby URLs list to resemble search results Add layouts from the Android Physical Web app that display URL items with blue page titles, green URLs, gray description text, and favicon. Also add NearbyUrlsAdapter for displaying PwsResults with these layouts. BUG=529962 ========== to ========== Modify Physical Web Nearby URLs list to resemble search results Add layouts from the Android Physical Web app that display URL items with blue page titles, green URLs, gray description text, and favicon. Also add NearbyUrlsAdapter for displaying PwsResults with these layouts. BUG=529962 ==========
mattreynolds@chromium.org changed reviewers: + agrieve@chromium.org, dvh@chromium.org
https://codereview.chromium.org/1412423007/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/NearbyUrlsAdapter.java (right): https://codereview.chromium.org/1412423007/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/NearbyUrlsAdapter.java:52: iconImageView.setImageDrawable(null); FYI, you'll probably get as input a Bitmap and the method to set a bitmap is setImageBitmap() https://codereview.chromium.org/1412423007/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/1412423007/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:2293: <message name="IDS_PHYSICAL_WEB_FAVICON_CONTENT_DESCRIPTION" desc="Content description of the favicon field for a discovered URL."> I'm not sure about adding that string. I think that "android:contentDescription" is for accessibility and it's useful where there's something that needs to be described to the user and I think that's not useful to give a generic description of the favicon to the user with vision disabilities. I think we can remove that string and the "android:contentDescription" attribute altogether.
The CQ bit was checked by mattreynolds@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412423007/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412423007/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by dvh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412423007/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412423007/1
> I think we can remove that string and the "android:contentDescription" attribute > altogether. Removing it causes the build to fail with an accessibility lint warning: chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:55 [Accessibility] Missing `contentDescription` attribute on image: ContentDescription [warning] I agree the current description string isn't very helpful since all favicons would have the same (useless) description. I'll check how we set favicon content descriptions elsewhere in chromium.
lgtm with nits https://codereview.chromium.org/1412423007/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml (right): https://codereview.chromium.org/1412423007/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:3: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" Might be worth seeing if a RelativeLayout would allow for fewer nodes to be used here. http://android-developers.blogspot.ca/2009/02/android-layout-tricks-1.html https://codereview.chromium.org/1412423007/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/NearbyUrlsAdapter.java (right): https://codereview.chromium.org/1412423007/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/NearbyUrlsAdapter.java:20: public class NearbyUrlsAdapter extends ArrayAdapter<PwsResult> { nit: make package-private?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> FYI, you'll probably get as input a Bitmap and the method to set a bitmap is
setImageBitmap()
Done
> I think we can remove that string and the "android:contentDescription"
attribute
altogether.
Looks like we set contentDescription to @null in other situations where we have
favicons, see bookmark_thumbnail_widget_item_folder.xml for an example. I
couldn't get Chrome to "read" any favicons to me in Talk Back so it's probably
correct?
> Might be worth seeing if a RelativeLayout would allow for fewer nodes to be
used
> here.
Done (RelativeLayout is awesome, thanks for the heads-up)
> public class NearbyUrlsAdapter extends ArrayAdapter<PwsResult> {
> nit: make package-private?
Done
The CQ bit was checked by mattreynolds@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412423007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412423007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/10/22 23:44:51, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. lgtm.
dvh@chromium.org changed reviewers: + mariakhomenko@chromium.org
mariakhomenko@chromium.org: Please review changes in Please could you take a look? Thanks!
dvh@chromium.org changed reviewers: + yusufo@chromium.org
yusufo@chromium.org: Could you please take a look at chrome/android/java/res/*? Thanks!
https://codereview.chromium.org/1412423007/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml (right): https://codereview.chromium.org/1412423007/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:10: android:background="#FFFFFF"> nit: lowercase https://codereview.chromium.org/1412423007/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:12: android:id="@+id/icon" these id names are generic, please add some sort of physical web prefix to them. https://codereview.chromium.org/1412423007/diff/20001/chrome/android/java/res... File chrome/android/java/res/layout/physical_web_list_urls_activity.xml (right): https://codereview.chromium.org/1412423007/diff/20001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_urls_activity.xml:10: android:id="@android:id/list" same comment as last file https://codereview.chromium.org/1412423007/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java (right): https://codereview.chromium.org/1412423007/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:68: intent.setData(Uri.parse(url)); you probably want to add category Intent.CATEGORY_BROWSABLE to these intents.
The CQ bit was checked by mattreynolds@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/1412423007/#ps20001 (title: "wip (taking changes from dvh,agrieve)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412423007/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412423007/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by mattreynolds@chromium.org
Thanks Maria!
> android:background="#FFFFFF">
> nit: lowercase
Done
> android:id="@+id/icon"
> these id names are generic, please add some sort of physical web prefix to
them.
Done
> android:id="@android:id/list"
> same comment as last file
I think this is required to be id/list by the ListAdapter. Changing it gives me
an error message:
java.lang.RuntimeException: Unable to start activity
ComponentInfo{com.google.android.apps.chrome/org.chromium.chrome.browser.physicalweb.ListUrlsActivity}:
java.lang.RuntimeException: Your content must have a ListView whose id attribute
is 'android.R.id.list'
> intent.setData(Uri.parse(url));
> you probably want to add category Intent.CATEGORY_BROWSABLE to these intents.
Done
lgtm
mattreynolds@chromium.org changed reviewers: + aurimas@chromium.org
aurimas@chromium.org Please take a look at the res changes in chrome/android/java/res/*, thanks!
Is there a mock of what the UI should look like? https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml (right): https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:1: <?xml version="1.0" encoding="utf-8"?> Missing copyright header. See other xml files. https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:8: android:layout_width="fill_parent" s/fill_parent/match_parent fill_parent is is deprecated. https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:27: android:text="" This does nothing. Please remove. https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:36: android:layout_width="fill_parent" s/fill_parent/match_parent fill_parent is is deprecated. https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:41: android:text="" This does nothing. Please remove. https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:50: android:layout_width="fill_parent" s/fill_parent/match_parent fill_parent is is deprecated. https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:53: android:layout_alignParentStart="true" You cannot align by both parent start and parent end. Remove one of them. https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:55: android:text="" This does nothing. Please remove. https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:57: android:fontFamily="arial-sans-serif" Roboto is a standard font in Android. Arial is not used on Android. Also no need to set sans-serif which is default already. https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... File chrome/android/java/res/layout/physical_web_list_urls_activity.xml (right): https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_urls_activity.xml:1: <?xml version="1.0" encoding="utf-8"?> Missing copyright header. Check other xml files for example https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_urls_activity.xml:7: android:background="#ffffffff"> It seems like you want white to be the background everywhere. Instead of setting the background to white here and in physical_web_list_item_nearby_url.xml, please set it at the activity level using styles. <item name="android:windowBackground">#fff</item> Also keep in mind that alpha is 100% by default.
> Is there a mock of what the UI should look like? There's a mock of the Nearby URLs UI on slide 2 of this deck: https://docs.google.com/presentation/d/1qFysFB5p1Dg9uxiTmH_PtFXzFX_XpzdUeLzy3...
> Missing copyright header. See other xml files. Done x2 > fill_parent is is deprecated. Done x4 > android:text="" > This does nothing. Please remove. Done x3 > android:layout_alignParentStart="true" > You cannot align by both parent start and parent end. Remove one of them. Removed android:layout_alignParentEnd > android:fontFamily="arial-sans-serif" > Roboto is a standard font in Android. Arial is not used on Android. Also no need > to set sans-serif which is default already. Removed all three android:fontFamily attrs so we'll use the default > https://codereview.chromium.org/1412423007/diff/40001/chrome/android/java/res... > chrome/android/java/res/layout/physical_web_list_urls_activity.xml:7: > android:background="#ffffffff"> > It seems like you want white to be the background everywhere. Instead of setting > the background to white here and in physical_web_list_item_nearby_url.xml, > please set it at the activity level using styles. > <item name="android:windowBackground">#fff</item> > > Also keep in mind that alpha is 100% by default. Setting android:windowBackground to a hex color value produces an error: Error: Color types not allowed (at 'android:windowBackground' with value '#fff'). Adding a new colors.xml value works.
Can you post a screenshot of what UI looks like after this patch? https://codereview.chromium.org/1412423007/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml (right): https://codereview.chromium.org/1412423007/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:7: android:paddingStart="18dp" Since all the paddings are the same you can simply use: android:padding="18dp" https://codereview.chromium.org/1412423007/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:31: android:maxLines="1" Use android:singleLine="true" maxLines has bugs in certain versions of Android. https://codereview.chromium.org/1412423007/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java (right): https://codereview.chromium.org/1412423007/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/physicalweb/ListUrlsActivity.java:68: intent.addCategory(Intent.CATEGORY_BROWSABLE); We will allow any browser to use open these intents? Should we be hard-coding Chrome since it is part of Chrome?
> Can you post a screenshot of what UI looks like after this patch? Here's the current display with these changes: https://x20web.corp.google.com/~mattreynolds/screenshots/clank_nearbyurls_nom... Page title, description, and favicon are not yet implemented so only the URL is displayed. This version uses dummy values for title and description: https://x20web.corp.google.com/~mattreynolds/screenshots/clank_nearbyurls_fak... The inclusion and positioning of the favicon is still under discussion. > android:paddingStart="18dp" > Since all the paddings are the same you can simply use: > android:padding="18dp" Done > android:maxLines="1" > Use android:singleLine="true" > > maxLines has bugs in certain versions of Android. Done > intent.addCategory(Intent.CATEGORY_BROWSABLE); > We will allow any browser to use open these intents? Should we be hard-coding > Chrome since it is part of Chrome? Although Nearby URLs is part of Chrome, it's exposed through a notification and not the browser itself. Given that the user didn't start the activity from the Chrome app, it seems appropriate to allow the user to choose which browser handles the intent.
lgtm https://chromiumcodereview.appspot.com/1412423007/diff/80001/chrome/android/j... File chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml (right): https://chromiumcodereview.appspot.com/1412423007/diff/80001/chrome/android/j... chrome/android/java/res/layout/physical_web_list_item_nearby_url.xml:28: android:maxLines="1" Change this to singleLine="true" like below.
> android:maxLines="1" > Change this to singleLine="true" like below. Done. Thanks Aurimas!
The CQ bit was checked by mattreynolds@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dvh@chromium.org, agrieve@chromium.org, mariakhomenko@chromium.org, aurimas@chromium.org Link to the patchset: https://codereview.chromium.org/1412423007/#ps100001 (title: "wip (fix maxLines->singleLine)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412423007/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412423007/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/b2f96a5eefb9ca66435bba93495b3cddcdcaa80d Cr-Commit-Position: refs/heads/master@{#356160} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
