Closed Bug 1328837 Opened 7 years ago Closed 7 years ago

Move 'Switchboard' code into our module/package

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: sebastian, Assigned: swaroop.rao)

References

Details

Attachments

(1 file)

Bug 1328837 - Move 'Switchboard' code into our module/package. Refactored SwitchBoard component to pull it into the app module.
59 bytes, text/x-review-board-request
sebastian
: review+
Details 
Currently the Switchboard code lives in the third-party module. However since landing the library it has been heavily modified and has a completely different workflow than the original Switchboard library had:

* We are synchronizing a experiment configuration from a Kinto instance
* We can have local overrides of experiments
* We do the "bucketing" on the client side and therefore do not require a Switchboard server instance - and do not need to send potentially sensitive information (a unique user id) to a server

So I guess it's time to move this code into our module and package - and maybe even replace the name "Switchboard" with something different. Moving the code will allow us to use our utility methods and not need to re-implement functionality (e.g. using the device's proxy - bug 1324427).
Assignee: nobody → swaroop.rao
Status: NEW → ASSIGNED
Comment on attachment 8824168 [details]
Bug 1328837 - Move 'Switchboard' code into our module/package. Refactored SwitchBoard component to pull it into the app module.

https://reviewboard.mozilla.org/r/102698/#review104162

This looks good to me in general. However before landing i'd like to flag another person to look at the licensing.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
(Diff revision 1)
>  import org.mozilla.gecko.prompts.Prompt;
>  import org.mozilla.gecko.reader.SavedReaderViewHelper;
>  import org.mozilla.gecko.reader.ReaderModeUtils;
>  import org.mozilla.gecko.reader.ReadingListHelper;
>  import org.mozilla.gecko.restrictions.Restrictable;
> -import org.mozilla.gecko.restrictions.RestrictedProfileConfiguration;

There are some unrelated import changes here (RestrictedProfileConfiguration, BundleEventListener, HashMap). I suggest removing them from the patch to avoid conflicts with other changes.
Attachment #8824168 - Flags: review?(s.kaspari)
Comment on attachment 8824168 [details]
Bug 1328837 - Move 'Switchboard' code into our module/package. Refactored SwitchBoard component to pull it into the app module.

Hey Gervase, I'm not sure if you are the right person, but if not then you probably know who is: This 'switchboard' code used to live in a third-party module - since then we have refactored several parts and it works completely different now. To make this code use our utility methods etc. and access other parts of our code base we would like to move this now into our main module. The original switchboard code has an Apache License code and I'm wondering if we should or need to update the file headers - and can or should this be re-licensed as MPL?
Attachment #8824168 - Flags: feedback?(gerv)
I have submitted a new patch for review. I don't really know the best way to re-submit the same patch with additional changes. I read somewhere that you can do 'hg commit --amend' to make changes to an earlier commit, so I tried doing that. I did a quick review of the changes and I also did a 'mach build' to ensure I hadn't broken anything.
Comment on attachment 8824168 [details]
Bug 1328837 - Move 'Switchboard' code into our module/package. Refactored SwitchBoard component to pull it into the app module.

See comment 3.
Attachment #8824168 - Flags: feedback?(gerv)
Comment on attachment 8824168 [details]
Bug 1328837 - Move 'Switchboard' code into our module/package. Refactored SwitchBoard component to pull it into the app module.

https://reviewboard.mozilla.org/r/102698/#review105278

This is looking good so far. But the patch only contains the modified files and not actually moving the switchboard files.

You might need to make mercurial aware of the move.

See your local state with:
> hg st

then move files with:
> hg mv path/path/old path/path/new

Or if the file is already moved then with
> hg mv --after path/path/old path/path/new

After that update your commit:
> hg commit --amend

And then push again. Thanks!
Attachment #8824168 - Flags: review?(s.kaspari) → review-
(In reply to Sebastian Kaspari (:sebastian) from comment #3)
> Hey Gervase, I'm not sure if you are the right person, but if not then you
> probably know who is: This 'switchboard' code used to live in a third-party
> module - since then we have refactored several parts and it works completely
> different now. To make this code use our utility methods etc. and access
> other parts of our code base we would like to move this now into our main
> module. The original switchboard code has an Apache License code and I'm
> wondering if we should or need to update the file headers - and can or
> should this be re-licensed as MPL?

No, you can't strip the license off and change it to MPL without the permission of the copyright holder. Apache is fine in Mozilla code, so no need to worry about that. Please keep the code in its own directory (even if it's no longer in the third-party section) with a copy of the Apache license in a LICENSE file.

Gerv
Attachment #8824168 - Flags: feedback?(gerv)
Attachment #8824168 - Flags: review- → review?(s.kaspari)
(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> Try run:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=25448608e266

Let's see if this passes all tests.

Additionally it seems like this patch needs to be rebased in order to land (there are some conflicts in BrowserApp and ActivityStream) - can you do that and upload a new version of the patch?
Flags: needinfo?(swaroop.rao)
Sure thing. I'll do that later today and submit the patch.
Flags: needinfo?(swaroop.rao)
The try run was successful. So it looks like we can land this - after rebasing. :)
Comment on attachment 8824168 [details]
Bug 1328837 - Move 'Switchboard' code into our module/package. Refactored SwitchBoard component to pull it into the app module.

I'll clear review here. Just re-flag me for the new patch. Thanks!
Attachment #8824168 - Flags: review?(s.kaspari)
I didn't get any response on IRC to my question about doing a merge and resolving conflicts, so I tried my best to follow instructions I found on the web and did it in the best way I could. I have uploaded a patch for review.
Comment on attachment 8824168 [details]
Bug 1328837 - Move 'Switchboard' code into our module/package. Refactored SwitchBoard component to pull it into the app module.

https://reviewboard.mozilla.org/r/102698/#review107934

Something went terribly wrong with this patch. It now removes a lot of code that has been added recently.

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
(Diff revision 3)
> -
> -            case "Website:AppInstalled":
> -                final String name = message.getString("name");
> -                final String startUrl = message.getString("start_url");
> -                final Bitmap icon = FaviconDecoder
> -                    .decodeDataURI(getContext(), message.getString("icon"))
> -                    .getBestBitmap(GeckoAppShell.getPreferredIconSize());
> -                createShortcut(name, startUrl, icon);
>                  break;

For example here..
Attachment #8824168 - Flags: review?(s.kaspari) → review-
It became too difficult to figure out what went wrong and fix it, so I just ended up blowing away my local copy and got a fresh copy of the code and made the necessary changes. I've uploaded a new patch for review.
Comment on attachment 8824168 [details]
Bug 1328837 - Move 'Switchboard' code into our module/package. Refactored SwitchBoard component to pull it into the app module.

https://reviewboard.mozilla.org/r/102698/#review109230

LGTM. Thanks for updating the patch!
Attachment #8824168 - Flags: review?(s.kaspari) → review+
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4424944b72fc
Move 'Switchboard' code into our module/package. Refactored SwitchBoard component to pull it into the app module. r=sebastian
So, Sebastian, should I wait for this to land before I work on https://bugzilla.mozilla.org/show_bug.cgi?id=1324427? How will I know when the changes have been applied to the code repository?
Flags: needinfo?(s.kaspari)
I landed the code in the autoland repository. It will be merged to mozilla-central in the next 24 hours. This bug will be closed automatically when this happens.

Either wait for that or start to create your new patch on top of your local changes.
Flags: needinfo?(s.kaspari)
https://hg.mozilla.org/mozilla-central/rev/4424944b72fc
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: