Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2810973003:
Remove maybeCreatePermissionForDefaultSearchEngine and related code (Closed)
DescriptionRemove maybeCreatePermissionForDefaultSearchEngine and related code
The function maybeCreatePermissionForDefaultSearchEngine was introduced
during development of a feature for M43, but all calls to the function were
removed in the same milestone.
In more detail, the function was introduced in revision
493f0ea4a1752dcdd355323426007c9077a6e84b, which was in M43. The
last call to the function was removed in revision
ca5a45d777d9f6ca14a4e41d262268cc5139aace, which was merged into M43.
See https://crbug.com/467798 for more details / CLs.
One of the side effects of the function is to set a preference
LOCATION_AUTO_ALLOWED when setting permissions. This preference is
checked in another function, and if present the permission is cleared. As the
function was never in a released version, this preference can't be set, so
the related code checking this preference can also be removed.
This further allows a boolean parameter to be removed from another function,
as the parameter is always true except for when it was called by the related code
that is being removed.
I'm removing all of this now as it adds confusing complexity to fixing
a bug I'm currently looking at.
BUG=703516
Review-Url: https://codereview.chromium.org/2810973003
Cr-Commit-Position: refs/heads/master@{#463921}
Committed: https://chromium.googlesource.com/chromium/src/+/b2197feae09d55e93d632fef6292729b1655ef1a
Patch Set 1 #Patch Set 2 : Remove more of the stuff #
Total comments: 2
Patch Set 3 : Remove strign #
Messages
Total messages: 25 (18 generated)
The CQ bit was checked by benwells@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by benwells@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove unused function The function was introduced during development of a feature. BUG=703516 ========== to ========== Remove unused function maybeCreatePermissionForDefaultSearchEngine The function was introduced during development of a feature for M43, but all calls to the function were removed in the same milestone. In more detail, the function was introduced in revision 493f0ea4a1752dcdd355323426007c9077a6e84b, which was in M43. The last call to the function was removed in revision ca5a45d777d9f6ca14a4e41d262268cc5139aace, which was merged into M43. See https://crbug.com/467798 for more details. I'm removing it now as it adds confusing complexity to fixing the bug I'm currently looking at. BUG=703516 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
benwells@chromium.org changed reviewers: + finnur@chromium.org
The CL description does not accurately reflect what is being removed. It describes one unused function is being removed (good), but does not talk about the removal of code that, on the face of it, seems to be used. Was it not supposed to be included in this CL? If this is intentional I'm not sure about the correctness of all of this, since it has been quite a while, so I don't remember the flow anymore. Can you expand on the CL description and perhaps go into a bit more details in the comments why it is safe to perform the Adapter changes?
Description was changed from ========== Remove unused function maybeCreatePermissionForDefaultSearchEngine The function was introduced during development of a feature for M43, but all calls to the function were removed in the same milestone. In more detail, the function was introduced in revision 493f0ea4a1752dcdd355323426007c9077a6e84b, which was in M43. The last call to the function was removed in revision ca5a45d777d9f6ca14a4e41d262268cc5139aace, which was merged into M43. See https://crbug.com/467798 for more details. I'm removing it now as it adds confusing complexity to fixing the bug I'm currently looking at. BUG=703516 ========== to ========== Remove maybeCreatePermissionForDefaultSearchEngine and related code The function maybeCreatePermissionForDefaultSearchEngine was introduced during development of a feature for M43, but all calls to the function were removed in the same milestone. In more detail, the function was introduced in revision 493f0ea4a1752dcdd355323426007c9077a6e84b, which was in M43. The last call to the function was removed in revision ca5a45d777d9f6ca14a4e41d262268cc5139aace, which was merged into M43. See https://crbug.com/467798 for more details / CLs. One of the side effects of the function is to set a preference LOCATION_AUTO_ALLOWED when setting permissions. This preference is checked in another function, and if present the permission is cleared. As the function was never in a released version, this preference can't be set, so the related code checking this preference can also be removed. This further allows a boolean parameter to be removed from another function, as the parameter is always true except for when it was called by the related code that is being removed. I'm removing all of this now as it adds confusing complexity to fixing a bug I'm currently looking at. BUG=703516 ==========
Description was changed from ========== Remove maybeCreatePermissionForDefaultSearchEngine and related code The function maybeCreatePermissionForDefaultSearchEngine was introduced during development of a feature for M43, but all calls to the function were removed in the same milestone. In more detail, the function was introduced in revision 493f0ea4a1752dcdd355323426007c9077a6e84b, which was in M43. The last call to the function was removed in revision ca5a45d777d9f6ca14a4e41d262268cc5139aace, which was merged into M43. See https://crbug.com/467798 for more details / CLs. One of the side effects of the function is to set a preference LOCATION_AUTO_ALLOWED when setting permissions. This preference is checked in another function, and if present the permission is cleared. As the function was never in a released version, this preference can't be set, so the related code checking this preference can also be removed. This further allows a boolean parameter to be removed from another function, as the parameter is always true except for when it was called by the related code that is being removed. I'm removing all of this now as it adds confusing complexity to fixing a bug I'm currently looking at. BUG=703516 ========== to ========== Remove maybeCreatePermissionForDefaultSearchEngine and related code The function maybeCreatePermissionForDefaultSearchEngine was introduced during development of a feature for M43, but all calls to the function were removed in the same milestone. In more detail, the function was introduced in revision 493f0ea4a1752dcdd355323426007c9077a6e84b, which was in M43. The last call to the function was removed in revision ca5a45d777d9f6ca14a4e41d262268cc5139aace, which was merged into M43. See https://crbug.com/467798 for more details / CLs. One of the side effects of the function is to set a preference LOCATION_AUTO_ALLOWED when setting permissions. This preference is checked in another function, and if present the permission is cleared. As the function was never in a released version, this preference can't be set, so the related code checking this preference can also be removed. This further allows a boolean parameter to be removed from another function, as the parameter is always true except for when it was called by the related code that is being removed. I'm removing all of this now as it adds confusing complexity to fixing a bug I'm currently looking at. BUG=703516 ==========
On 2017/04/11 10:21:42, Finnur wrote: > The CL description does not accurately reflect what is being removed. It > describes one unused function is being removed (good), but does not talk about > the removal of code that, on the face of it, seems to be used. Was it not > supposed to be included in this CL? If this is intentional I'm not sure about > the correctness of all of this, since it has been quite a while, so I don't > remember the flow anymore. Can you expand on the CL description and perhaps go > into a bit more details in the comments why it is safe to perform the Adapter > changes? It's all intentional. Sorry about the lack of details :/ I've expanded the CL description a fair bit, let me know if it doesn't make sense / is incomplete.
https://codereview.chromium.org/2810973003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/2810973003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:59: public static final String LOCATION_AUTO_ALLOWED = "search_engine_location_auto_allowed"; I just realised this can be removed as well, will do so with the next patch set.
Thanks for the explanation. LGTM. https://codereview.chromium.org/2810973003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/2810973003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:59: public static final String LOCATION_AUTO_ALLOWED = "search_engine_location_auto_allowed"; If this is the only additional line that will be removed, feel free to do it as part of this CL.
The CQ bit was checked by benwells@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by benwells@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org Link to the patchset: https://codereview.chromium.org/2810973003/#ps40001 (title: "Remove strign")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491967514540130,
"parent_rev": "dfe0ed4a9362cae34fe04555791f34b05cb21457", "commit_rev":
"b2197feae09d55e93d632fef6292729b1655ef1a"}
Message was sent while issue was closed.
Description was changed from ========== Remove maybeCreatePermissionForDefaultSearchEngine and related code The function maybeCreatePermissionForDefaultSearchEngine was introduced during development of a feature for M43, but all calls to the function were removed in the same milestone. In more detail, the function was introduced in revision 493f0ea4a1752dcdd355323426007c9077a6e84b, which was in M43. The last call to the function was removed in revision ca5a45d777d9f6ca14a4e41d262268cc5139aace, which was merged into M43. See https://crbug.com/467798 for more details / CLs. One of the side effects of the function is to set a preference LOCATION_AUTO_ALLOWED when setting permissions. This preference is checked in another function, and if present the permission is cleared. As the function was never in a released version, this preference can't be set, so the related code checking this preference can also be removed. This further allows a boolean parameter to be removed from another function, as the parameter is always true except for when it was called by the related code that is being removed. I'm removing all of this now as it adds confusing complexity to fixing a bug I'm currently looking at. BUG=703516 ========== to ========== Remove maybeCreatePermissionForDefaultSearchEngine and related code The function maybeCreatePermissionForDefaultSearchEngine was introduced during development of a feature for M43, but all calls to the function were removed in the same milestone. In more detail, the function was introduced in revision 493f0ea4a1752dcdd355323426007c9077a6e84b, which was in M43. The last call to the function was removed in revision ca5a45d777d9f6ca14a4e41d262268cc5139aace, which was merged into M43. See https://crbug.com/467798 for more details / CLs. One of the side effects of the function is to set a preference LOCATION_AUTO_ALLOWED when setting permissions. This preference is checked in another function, and if present the permission is cleared. As the function was never in a released version, this preference can't be set, so the related code checking this preference can also be removed. This further allows a boolean parameter to be removed from another function, as the parameter is always true except for when it was called by the related code that is being removed. I'm removing all of this now as it adds confusing complexity to fixing a bug I'm currently looking at. BUG=703516 Review-Url: https://codereview.chromium.org/2810973003 Cr-Commit-Position: refs/heads/master@{#463921} Committed: https://chromium.googlesource.com/chromium/src/+/b2197feae09d55e93d632fef6292... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/b2197feae09d55e93d632fef6292... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
