1252960 - Rename UrlMetadata* table to UrlImageData*

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The UrlAnnotations table is more like the "UrlMetadata" table – let's rename the table for the data that's in it (url -> thumbnail metadata). For simplicity, let's change the names in code (e.g. class name) and not the actual table name. Add a comment to explain why the table name does not match the class names.

Note that following the code paths here might be a little tricky. Be sure to update the list of file names in the mobile/android/base/moz.build file too!

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

What about UrlImagedata ? We store potential thumbnails (from ms-tile) and touch icons. Just wanted to not tie it too much to thumbnails, since we also have a Thumbnail table, IIRC.

Comment 2

[rychljir]

Hi, I would like to fix this issue as my assignment at CTU Prague. Im newbie in open source, so I would appreciate any help I can get. I have some experiences with android, but just a few with repositories etc. Thank you in advance.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Hey – welcome to Bugzilla! Sure, we'll assign you once you post a patch! I'd recommend using an IDE for this one as it'll make your work a lot simpler.

To start, set up a build environment - you can see the instructions here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_for_Android_build

Then, you'll need to create a patch to upload - see
https://wiki.mozilla.org/Mobile/Fennec/Android/Suggested_workflow#Creating_commits_and_submitting_patches

You can find more miscellaneous information at: https://wiki.mozilla.org/Mobile/Fennec/Android

If you need any help, you can reply to this bug, or feel free to message me on IRC - my nick is "mcomella" and you can find me in #mobile. If you need IRC setup instructions, see https://wiki.mozilla.org/IRC

Thanks and happy coding! ^_^

Comment 4

[rychljir]

Attachment: Rename UrlMetadata* table to UrlImageData*

Comment 5

[rychljir]

Michael, can you please review my attached 
patch please? Im not sure if its completely correct. 
Thanks.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8729846 [details] [diff] [review]
Rename UrlMetadata* table to UrlImageData*

Review of attachment 8729846 [details] [diff] [review]:
-----------------------------------------------------------------

This could land, with exception to the moz.build file order.

Sorry for the delay rychljir – I mismanaged my time over the past week or so. The turn-around time will be much shorter next time around!

::: mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java
@@ -16,5 @@
>  import java.io.PipedInputStream;
>  import java.io.PipedOutputStream;
>  import java.net.MalformedURLException;
>  import java.net.Proxy;
> -import java.net.URI;

fwiw, we try not to make unrelated changes (like unused imports) in the same patch because it can make uplifting patches more difficult.

It's fine this time, but try not to do it in the future (and don't feel the need to go back and fix this with the other changes)!

::: mobile/android/base/java/org/mozilla/gecko/db/UrlImageDataTable.java
@@ +13,5 @@
> +import android.net.Uri;
> +
> +// Holds metadata info about urls. Supports some helper functions for getting back a HashMap of key value data.
> +
> +public class UrlImageDataTable extends BaseTable {

I apologize for misleading you initially – I was told yesterday that my UrlAnnotations class was not URLAnnotations so this rename should really be URLImageDataTable.

That being said, if it's too much trouble (and trust me, I understand!), we can leave it as "Url" and file a follow-up bug for someone else!

@@ +16,5 @@
> +
> +public class UrlImageDataTable extends BaseTable {
> +    private static final String LOGTAG = "GeckoURLMetadataTable";
> +
> +    //The class name was refactored for a better clarity and doesnt currently match the table name

nit: `// The class...`

Nice comment!

::: mobile/android/base/moz.build
@@ +244,5 @@
>      'db/TabsAccessor.java',
>      'db/TabsProvider.java',
>      'db/UrlAnnotations.java',
>      'db/URLMetadata.java',
> +    'db/UrlImageDataTable.java',

I think this would fail to build via `mach build` – the file names need to be in alphabetical order!

fwiw, I'm pretty sure the unix `sort` utility gets it right if your editor supports it and you wanted to use it.

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

fwiw, there were a few other classes I had intended to be moved in this bug but I filed bug 1259267 to do those (to get this bug to land asap).

Comment 8

[rychljir]

Attachment: Rename UrlMetadata* table to UrlImageData*

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c1811ea613fe

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8734482 [details] [diff] [review]
Rename UrlMetadata* table to UrlImageData*

Review of attachment 8734482 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! Thanks rychljir!

I made a push to our try test servers (above).

Once the push goes green, you can add the "checkin-needed" keyword [1] to get your patch checked in. Note that all patches added via checkin-needed keyword need an associated green try run. Let me know if you need help reading the results.

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 11

[rychljir]

There are two errors, but I dont see how are they related to our patch. What to do next, please?

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Our test suites often get intermittent failures and this looks like one of them. To check, you can see if the suite has been re-run (i.e. there is another test suite with the same name on the same device) and if that was green. Additionally, if you click a failing test suite, you can look at the bar on the bottom of the screen and see if it matches any existing known intermittent failures, which are also listed on that bar.

This looks good to me and is ready for check-in!

Comment 13

[Carsten Book [:Tomcat]]

failed to apply:

Fetching... done
Parsing... done
adding 1252960 to series file
renamed 1252960 -> bug1252960.patch
applying bug1252960.patch
patching file mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java
Hunk #1 FAILED at 35
Hunk #2 FAILED at 843
2 out of 2 hunks FAILED -- saving rejects to file mobile/android/base/java/org/mozilla/gecko/GeckoAppShell.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1252960.patch

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Can you rebase your patch onto the latest pull from fx-team or mozilla-central, rychljir?

Comment 15

[rychljir]

Attachment: Rename UrlMetadata* table to UrlImageData*

Comment 16

[rychljir]

I hope it will land, this patch cost a lot of sweat. Hopefully I wont no longer restrain you :))

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8738699 [details] [diff] [review]
Rename UrlMetadata* table to UrlImageData*

Review of attachment 8738699 [details] [diff] [review]:
-----------------------------------------------------------------

This patch doesn't build for me – it looks like you're missing some changes in GeckoApp, TopSitesPanel, etc. Something must have gone wrong during the rebase – let me know if you need help doing this process.

::: mobile/android/base/java/org/mozilla/gecko/db/URLImageDataTable.java
@@ +12,5 @@
>  import android.database.sqlite.SQLiteDatabase;
>  import android.net.Uri;
>  
>  // Holds metadata info about urls. Supports some helper functions for getting back a HashMap of key value data.
> +public class URLImageDataTable extends BaseTable {

You need to update the moz.build file for this change.

The build system in the IDE (gradle) is independent of the build system we ship with (running `mach build && mach package`) so you can catch errors like these by running that command (but you shouldn't need to do it every time).

Comment 18

[Tushar Saini (:shatur)]

Attachment: Bug 1252960 - Rename UrlMetadata* table to UrlImageData*

Review commit: https://reviewboard.mozilla.org/r/107524/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107524/

Comment 19

[Andrzej Hunt :ahunt]

Comment on attachment 8830808 [details]
Bug 1252960 - Rename UrlMetadata* table to UrlImageData*

https://reviewboard.mozilla.org/r/107524/#review109278

This looks good - just make sure you correctly tag file moves in mercurial, since it's impossible to see what changed while a file was moving otherwise. (See my review comment below for instructions on how to fix it.)

::: mobile/android/base/java/org/mozilla/gecko/db/URLImageDataTable.java:1
(Diff revision 1)
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*- */

It looks like mercurial isn't aware you moved the file - we want to track that it's just a move/rename (and not a new file), which also makes changes between the old and new version visible.


Doing this is slightly tricky, option 1 is to:
hg strip -r -1 --keep # removes the commit, but keeps changes
hg mv --after <old_location> <new_location>"
hg commit # commit again (try to copy the original commit message, including the review ID)

Option 2 is to use hg histedit to edit the commit.

::: mobile/android/base/java/org/mozilla/gecko/db/URLImageDataTable.java:18
(Diff revision 1)
> +import android.net.Uri;
> +
> +// Holds metadata info about urls. Supports some helper functions for getting back a HashMap of key value data.
> +// The class name was refactored for a better clarity and doesnt currently match the table name
> +public class URLImageDataTable extends BaseTable {
> +    private static final String LOGTAG = "GeckoURLMetadataTable";

We probably want to update the name here? (This is just a tag that is used when printing log messages, so we can change it without issues.)

Comment 20

[Tushar Saini (:shatur)]

Comment on attachment 8830808 [details]
Bug 1252960 - Rename UrlMetadata* table to UrlImageData*

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107524/diff/1-2/

Comment 21

[Andrzej Hunt :ahunt]

Comment on attachment 8830808 [details]
Bug 1252960 - Rename UrlMetadata* table to UrlImageData*

https://reviewboard.mozilla.org/r/107524/#review111270

looks good to me! Thank you!

Comment 22

[Tushar Saini (:shatur)]

Hey Andrez!! 

  There is failed test shown on try. Is this relevant?? Also, can you please assign this bug to me?

Thanks!!

Comment 23

[Andrzej Hunt :ahunt]

Try link (for some reason it didn't get auto-posted):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73c0fa029e32c3f669861987852c00208881442

(In reply to Tushar Saini (:shatur) from comment #22)
> Hey Andrez!! 
> 
>   There is failed test shown on try. Is this relevant?? Also, can you please
> assign this bug to me?
> 
> Thanks!!

I've assigned the bug to you - sorry I never noticed that before.

The failing test is unrelated, and was fixed in Bug 1318544 - i.e. it's irrelevant here! This looks ready for landing (i.e. you'll probably want to set checkin-needed : ) )

Comment 24

[Tushar Saini (:shatur)]

Keywords: checkin-needed

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7038e10280b2
Rename UrlMetadata* table to UrlImageData* r=ahunt

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out because it seemed like it caused this test to intermittently fail: https://treeherder.mozilla.org/logviewer.html#?job_id=75304311&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/4f8438e85c65

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

These failures popped up elsewhere at the same time, so this patch shouldn't be the cause. I'll re-land once I sort out the failures.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af979611f4d1
Rename UrlMetadata* table to UrlImageData* r=ahunt

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/af979611f4d1