Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2250433003:
Replaces find pasteboard with a static variable in Find in page. (Closed)
|
Created:
4 years, 4 months ago by stkhapugin Modified:
4 years, 4 months ago Reviewers:
Eugene But (OOO till 7-30) CC:
chromium-reviews, sdefresne+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReplaces find pasteboard with a static variable in Find in page.
Since find pasteboards are unavailable on iOS 10, this CL removes usage
of UIFindPasteboard, replacing it with a simple static NSString variable
BUG=630563
Committed: https://crrev.com/27bb5b841b8ba1684666f6cd16b0640c443599d6
Cr-Commit-Position: refs/heads/master@{#412488}
Patch Set 1 #
Total comments: 14
Patch Set 2 : comments #Patch Set 3 : Release old value #Messages
Total messages: 14 (6 generated)
The CQ bit was checked by stkhapugin@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...
stkhapugin@chromium.org changed reviewers: + eugenebut@chromium.org
PTAL
lgtm https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... File ios/chrome/browser/find_in_page/find_in_page_controller.mm (right): https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:34: static NSString* staticSearchTerm; This should have comments per Objective-C Style Guide. Would be great to explain that variable is used for sharing search term between different tabs. https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:34: static NSString* staticSearchTerm; Please prefix globals with |g|, not with |static|: go/objc-style?showone=Variable_Names&cl=head#Variable_Names But maybe instead of global you should have a class static variable? https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:43: // Sets the search term to |string|. Stored until the application quit. Maybe move these comments to static variable so these comments could be as simple as "Backs up class variable of the same name"? https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:304: [[FindInPageController class] setSearchTerm:[[self findInPageModel] text]]; s/FindInPageController/self https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:314: NSString* term = [[FindInPageController class] searchTerm]; ditto https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:318: - (UIPasteboard*)findPasteboard { Is this still used?
Thank you for a quick review. https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... File ios/chrome/browser/find_in_page/find_in_page_controller.mm (right): https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:34: static NSString* staticSearchTerm; On 2016/08/16 16:34:45, Eugene But wrote: > Please prefix globals with |g|, not with |static|: > go/objc-style?showone=Variable_Names&cl=head#Variable_Names > > But maybe instead of global you should have a class static variable? There are no class variables in ObjC. Renaming done. https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:34: static NSString* staticSearchTerm; On 2016/08/16 16:34:44, Eugene But wrote: > This should have comments per Objective-C Style Guide. Would be great to explain > that variable is used for sharing search term between different tabs. Done. https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:43: // Sets the search term to |string|. Stored until the application quit. On 2016/08/16 16:34:45, Eugene But wrote: > Maybe move these comments to static variable so these comments could be as > simple as "Backs up class variable of the same name"? Since there are no class variables, I'll have to keep a free-floating static, I guess. Or do you mean @interface Blah () { /*ivar declaration list*/ } @end this? https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:304: [[FindInPageController class] setSearchTerm:[[self findInPageModel] text]]; On 2016/08/16 16:34:45, Eugene But wrote: > s/FindInPageController/self Done. https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:314: NSString* term = [[FindInPageController class] searchTerm]; On 2016/08/16 16:34:44, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:318: - (UIPasteboard*)findPasteboard { On 2016/08/16 16:34:44, Eugene But wrote: > Is this still used? No, good catch! Removed.
These are optional. https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... File ios/chrome/browser/find_in_page/find_in_page_controller.mm (right): https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:34: static NSString* staticSearchTerm; > There are no class variables in ObjC. Renaming done. This is a class variable, no? @interface FindInPageController () <DOMAltering, CRWWebStateObserver> { // comments... static NSString* _searchTerm; } @end https://codereview.chromium.org/2250433003/diff/1/ios/chrome/browser/find_in_... ios/chrome/browser/find_in_page/find_in_page_controller.mm:43: // Sets the search term to |string|. Stored until the application quit. On 2016/08/16 16:45:57, stkhapugin wrote: > On 2016/08/16 16:34:45, Eugene But wrote: > > Maybe move these comments to static variable so these comments could be as > > simple as "Backs up class variable of the same name"? > > Since there are no class variables, I'll have to keep a free-floating static, I > guess. Or do you mean @interface Blah () { /*ivar declaration list*/ } @end > this? Replied above about class variable. If you have global variable instead of class variable the comments could be "Backs up gSearchTerm variable of the same name." The point is to have comments in one place.
Turns out, this is not allowed in Objective-c:
@interface FindInPageController () <DOMAltering, CRWWebStateObserver> {
// ...
static NSString* _searchTerm;
}
Yields:
../../ios/chrome/browser/find_in_page/find_in_page_controller.mm:38:3: error:
type name does not allow storage class to be specified
And an attempt to access _searchTerm in class methods yields:
../../ios/chrome/browser/find_in_page/find_in_page_controller.mm:99:4: error:
instance variable '_searchTerm' accessed in class method
If you find a way of doing this without a regular static variable, I'll do it.
For now, pushing as is.
Thank you!
The CQ bit was checked by stkhapugin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2250433003/#ps40001 (title: "Release old value")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Replaces find pasteboard with a static variable in Find in page. Since find pasteboards are unavailable on iOS 10, this CL removes usage of UIFindPasteboard, replacing it with a simple static NSString variable BUG=630563 ========== to ========== Replaces find pasteboard with a static variable in Find in page. Since find pasteboards are unavailable on iOS 10, this CL removes usage of UIFindPasteboard, replacing it with a simple static NSString variable BUG=630563 Committed: https://crrev.com/27bb5b841b8ba1684666f6cd16b0640c443599d6 Cr-Commit-Position: refs/heads/master@{#412488} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/27bb5b841b8ba1684666f6cd16b0640c443599d6 Cr-Commit-Position: refs/heads/master@{#412488} |
