Closed Bug 907161 Opened 11 years ago Closed 11 years ago

Remove UA override for domains week starting 2013-08-19

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:leo+, b2g18 fixed, b2g-v1.1hd fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
b2g18 --- fixed
b2g-v1.1hd --- fixed

People

(Reporter: karlcow, Assigned: karlcow)

References

Details

Attachments

(1 file, 1 obsolete file)

This bug is the batch for the next UA override removal.
Adding pontofrio.com.br INVALID Bug 827624
Depends on: 827624
Depends on: 878266
remove  i-dizajn.com (Bug 878266)
Depends on: 827666
remove slideshare.net (Bug 827666)
Attached patch ua-override-20130830.patch (obsolete) — Splinter Review
This patch is intended to be applied on both 1.2 and 1.1
Attachment #797878 - Flags: review?(lmandel)
blocking-b2g: --- → leo?
Karl - Your patch removes the overrides for:
accounts.google.com
4shared.com
pontofrio.com.br
wp.pl
o2.pl
tvn24.pl
tablica.pl
as.com
elconfidencial.com
booking.com
rincondelvago.com
edmunds.com
aprod.hu
badoo.com
profession.hu
i-dizajn.com
flickr.com

It does not remove the override for slideshare.net (bug 827666), which is marked as a dependency. Can you please add a dependency on the evangelism bugs for the domains that you intend to remove from the override list so that I can confirm that the correct overrides have been removed?
Flags: needinfo?(kdubost)
Comment on attachment 797878 [details] [diff] [review]
ua-override-20130830.patch

r- I'm unclear which overrides should be removed but, based on the bug's dependencies, it seems that the removal of slideshare.net should be included in this patch.
Attachment #797878 - Flags: review?(lmandel) → review-
New patch
Flags: needinfo?(kdubost)
Attachment #798045 - Flags: review?(lmandel)
That was weird. Specifically when I had double checked slideshare.net. :/
So this time the patch should be ok. Sorry for the noise.
Comment on attachment 797878 [details] [diff] [review]
ua-override-20130830.patch

Marking as obsolete as Karl attached a new patch
Attachment #797878 - Attachment is obsolete: true
Karl - I now see slideshare.net included in the patch. Thanks. I would still like to confirm that list of domains that should be included in this patch. Can you please confirm the list in comment 5 and add the evangelism bugs as dependencies for this bug?
Flags: needinfo?(kdubost)
I think I misunderstood some of the goals. Let's try to clarify what happened to fix the mistake I might have made.

0. B2G master is 1.2, the last 3 UA override Pull requests were made on 1.2
1. We can remove the UA override for B2G 1.1
2. After discussions with fabrice, I understood that I can attach a patch and put leo as a reference and that it will be propagated to other branches 1.2 and the different 1.1 ones.
3. I created a patch which covers the previous Pull Request to now 
   git diff -U8 fb3506f > ua-override-20130830.patch
   (minor the slideshare issue I had forgotten)
4. Attached the patch

* What is missing? So I can fix it
* Or should I have done something different?
Flags: needinfo?(kdubost) → needinfo?(lmandel)
(In reply to Karl Dubost :karlcow from comment #11)
> * What is missing? So I can fix it

These patches are trivial. In reviewing I'm providing a sanity check on two items:
1. That the prefs file is still valid after each removal. i.e. That you have correctly removed a single line for a single override.
2. The the list of removed overrides is the intended/correct list.

For 2, I need some context in order to review. I currently gain that context about the list of override removals via the linked dependencies on this bug. That's how I caught that slideshare.net was not included in the original patch. The list of removed overrides in the current patch does not currently correspond with the list of dependent Tech Evangelism bugs. If the patch is correct, the following bugs should be linked as dependencies (from the prefs file):
bug 805164, bug 826502, bug 827624, bug 827666, bug 828351, bug 828356, bug 828362, bug 828374, bug 828383, bug 828397, bug 828420, bug 828435, bug 843146, bug 878220, bug 878224, bug 878226, bug 878251, bug 878266, bug 878638
Flags: needinfo?(lmandel)
Assignee: nobody → kdubost
Status: NEW → ASSIGNED
Flags: needinfo?(kdubost)
Ah understood now. Thanks for the clarification lawrence.
You want me to add them again, like the patches I had previously done for 1.2.
Flags: needinfo?(kdubost)
Comment on attachment 798045 [details] [diff] [review]
ua-override-20130830.patch

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

19 more UA overrides killed. \o/
Attachment #798045 - Flags: review?(lmandel) → review+
Open a Github PR and I'll merge it.
Ryan,

It has been done a few days ago.
https://github.com/mozilla-b2g/gaia/pull/11863

Thanks :)
Flags: needinfo?(ryanvm)
Thanks. For future reference, people often attach PR redirects to the bug.

Master: https://github.com/mozilla-b2g/gaia/commit/54cca89fa365d8f413fe73f6ec52b210655a60e4
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
Resolution: --- → FIXED
Well past the date of 8/19.
blocking-b2g: leo? → -
(In reply to Preeti Raghunath(:Preeti) from comment #18)
> Well past the date of 8/19.

Preeti, I think you've misunderstood this bug. We have a bug each week to remove the UA overrides for the sites fixed that week. Alex told me that we still have an opportunity to accept fixes in the 1.1 branch. As such, I asked Karl to nom his latest UA override changes. UA override removals are low risk code changes that benefit the end user by avoiding Android/iOS ads and improving market share statistics. I'm renomming based on the -leo comment 18.
blocking-b2g: - → leo?
leo+ based on low risk for clean up.
blocking-b2g: leo? → leo+
I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 54cca89fa365d8f413fe73f6ec52b210655a60e4
  <RESOLVE MERGE CONFLICTS>
  git commit
Flags: needinfo?(kdubost)
John,

on my local repo.

git fetch upstream
remote: Counting objects: 3412, done.
remote: Compressing objects: 100% (1549/1549), done.
remote: Total 2063 (delta 1414), reused 1072 (delta 496)
Receiving objects: 100% (2063/2063), 1.01 MiB | 518 KiB/s, done.
Resolving deltas: 100% (1414/1414), completed with 364 local objects.
From https://github.com/mozilla-b2g/gaia
   af587d3..98261d4  master     -> upstream/master
   577ab51..6324c3b  v1-train   -> upstream/v1-train
   3c42667..75228cd  v1.1.0hd   -> upstream/v1.1.0hd


→ git checkout v1-train
error: pathspec 'v1-train' did not match any file(s) known to git.

→ git status
# On branch master
nothing to commit (working directory clean)

→ git branch -a
* master
  ua-override-1.1
  remotes/origin/HEAD -> origin/master
  remotes/origin/master
  remotes/origin/ua-override-1.1
  remotes/origin/v1-train
  remotes/origin/v1.0.0
  remotes/origin/v1.0.1
  remotes/origin/v1.1.0hd
  remotes/upstream/master
  remotes/upstream/v1-train
  remotes/upstream/v1.0.0
  remotes/upstream/v1.0.1
  remotes/upstream/v1.1.0hd

→ git checkout remotes/origin/v1-train
Checking out files: 100% (2706/2706), done.
Note: checking out 'remotes/origin/v1-train'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

  git checkout -b new_branch_name

HEAD is now at 368489e... Merge pull request #10799 from mozilla-b2g/fix_vcard

→   git cherry-pick -x -m1 54cca89fa365d8f413fe73f6ec52b210655a60e4
warning: too many files (created: 250 deleted: 2020), skipping inexact rename detection
error: could not apply 54cca89... Merge pull request #11863 from karlcow/ua-override-1.1
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit -c 54cca89'

But this doesn't really make sense because ua-override-prefs.js is an "independent file" and the patch is only about the modification of this file.

John, did you try to just apply the patch and then commit? Or is there anything I can do.
Flags: needinfo?(kdubost) → needinfo?(jhford)
Hi Karl,

That script snippet assumes you already have a local v1-train branch.  This can be created with "git checkout -t origin/v1-train -b v1-train".

These UA override patches are almost always bitrotted, I end up just doing the diff by hand when it's time to uplift them as they never seem to apply cleanly.

[v1-train 58d0a1c]
Flags: needinfo?(jhford)
John - The good news is that we're moving to a dynamic UA override mechanism that will forgo the need for these patches at some point in the 1.2 cycle.
v1.1.0hd: 58d0a1c911185f387f4802827bc41607bc76593a
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: