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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Karl Dubost💡 :karlcow]

This bug is the batch for the next UA override removal.

Comment 1

[Karl Dubost💡 :karlcow]

Adding pontofrio.com.br INVALID Bug 827624

Comment 2

[Karl Dubost💡 :karlcow]

remove  i-dizajn.com (Bug 878266)

Comment 3

[Karl Dubost💡 :karlcow]

remove slideshare.net (Bug 827666)

Comment 4

[Karl Dubost💡 :karlcow]

Attachment: ua-override-20130830.patch

This patch is intended to be applied on both 1.2 and 1.1

Comment 5

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 7

[Karl Dubost💡 :karlcow]

Attachment: ua-override-20130830.patch

New patch

Comment 8

[Karl Dubost💡 :karlcow]

That was weird. Specifically when I had double checked slideshare.net. :/
So this time the patch should be ok. Sorry for the noise.

Comment 9

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 797878 [details] [diff] [review]
ua-override-20130830.patch

Marking as obsolete as Karl attached a new patch

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 11

[Karl Dubost💡 :karlcow]

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?

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

(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

Comment 13

[Karl Dubost💡 :karlcow]

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.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 798045 [details] [diff] [review]
ua-override-20130830.patch

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

19 more UA overrides killed. \o/

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Open a Github PR and I'll merge it.

Comment 16

[Karl Dubost💡 :karlcow]

Ryan,

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

Thanks :)

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Thanks. For future reference, people often attach PR redirects to the bug.

Master: https://github.com/mozilla-b2g/gaia/commit/54cca89fa365d8f413fe73f6ec52b210655a60e4

Comment 18

[Preeti Raghunath(:Preeti)]

Well past the date of 8/19.

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

(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.

Comment 20

[Preeti Raghunath(:Preeti)]

leo+ based on low risk for clean up.

Comment 21

[John Ford [:jhford] CET/CEST Berlin Time]

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

Comment 22

[Karl Dubost💡 :karlcow]

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.

Comment 23

[John Ford [:jhford] CET/CEST Berlin Time]

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]

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 58d0a1c911185f387f4802827bc41607bc76593a