Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10831269:
Extensions Docs Server: BranchUtility not fetching branch numbers correctly (fixed) (Closed)
|
Created:
8 years, 4 months ago by cduvall Modified:
8 years, 4 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionExtensions Docs Server: BranchUtility not fetching branch numbers correctly
BranchUtility was crashing when it tried to fetch the branch numbers for the
dev, beta, and stable channels. There was also a problem using the branch number
where the name should have been used.
ServerInstances are no longer leaked.
BUG=141909
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151386
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151401
Patch Set 1 #
Total comments: 15
Patch Set 2 : better testing and fixes #
Total comments: 6
Patch Set 3 : merge and fixes #Patch Set 4 : fix typo #Patch Set 5 : fix for index.htmls #
Messages
Total messages: 8 (0 generated)
This should be fixed before we launch server2.
https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... File chrome/common/extensions/docs/server2/branch_utility.py (right): https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... chrome/common/extensions/docs/server2/branch_utility.py:30: 'trunk' or 'local', then |channel_name| will be returned unchanged. "... because..." https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... File chrome/common/extensions/docs/server2/handler.py (right): https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... chrome/common/extensions/docs/server2/handler.py:110: for branch in ['dev', 'beta', 'stable', 'trunk', 'local']] currently only the branch utility knows what the actual branches are. Let's keep it that way and put this method on branch utility (like GetAllBranchNumbers). https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... chrome/common/extensions/docs/server2/handler.py:130: channel_name, why can't this go in the constructor of template_data_source? I.e. pass into _GetInstanceForBranch and thread through like that. In fact you wouldn't need to pass branch into there, you could make _GetInstanceForBranch figure it out for itself. https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... File chrome/common/extensions/docs/server2/integration_test.py (right): https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... chrome/common/extensions/docs/server2/integration_test.py:20: _FakeGetBranchNumberForChannelName) I alluded to something like this in a bug report I just replied to. A more integration-test kind of thing to do would be to set up a fake server which knows about the protocol. I.e. a fake omaha server kind of thing (you can just make it return canned responses that you get from omaha). The trick is getting the server to actually point to that "server" instead of the real server. I'd try doing this by putting the knowledge in the fake urlfetch implementation (in appengine_wrappers). Rather than that returning a FakeUrlFetch, allow it to be configured. I'm imagining an interface like --- integration_test.py import appengine_wrappers import url_constants class FakeOmahaProxy(object): def fetch(self, url): return "insert_canned_response_here" class FakeSubversionServer(object): _base_pattern = re.compile(r"chrome/common/extensions/(.*)$") def fetch(self, url): # Only the interesting part of the path. # ... whatever the local filesystem fetch is. return local_fetch('chrome/common/extensions/%s' % _base_pattern.match(url).groups()[0]) appengine_wrappers.ConfigureFakeUrlFetch({ url_constants.OMAHA_PROXY_URL: FakeOmahaProxy(), "%s/*" % url_constants.SVN_URL: FakeSubversionServer(), }) --- appengine_wrappers.py def ConfigureFakeUrlFetch(configuration): FAKE_URL_FETCHER_CONFIGURATION = configuration try: import ... except ImportError: class FakeUrlFetch(object): def fetch(self, url): # TODO: match globs. return FAKE_URL_FETCHER_CONFIGURATION[url].fetch(url) def create_rpc(self): # Return object that eventually just calls fetch() # so that the fake fetch implementations can handle it. def make_fetch_call(self): # ditto https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... File chrome/common/extensions/docs/server2/template_data_source.py (right): https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... chrome/common/extensions/docs/server2/template_data_source.py:59: def Create(self, request, channel_name): See comment in handler.py. This could go in the factory constructor right? https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... chrome/common/extensions/docs/server2/template_data_source.py:95: self._branch_info['current'] = channel_name If you do that, then you could make the current channel a parameter of _MakeBranchDict. https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... chrome/common/extensions/docs/server2/template_data_source.py:97: (('/' + channel_name) if channel_name != 'local' else '') + '/static') Will this cause problems when we need to configure what empty-path points to? Perhaps the static resource path should be yet-another-thing to dependency inject.
https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... File chrome/common/extensions/docs/server2/template_data_source.py (right): https://chromiumcodereview.appspot.com/10831269/diff/1/chrome/common/extensio... chrome/common/extensions/docs/server2/template_data_source.py:97: (('/' + channel_name) if channel_name != 'local' else '') + '/static') On 2012/08/13 00:53:00, kalman wrote: > Will this cause problems when we need to configure what empty-path points to? > > Perhaps the static resource path should be yet-another-thing to dependency > inject. I also note that SamplesDataSource does the same thing.
http://codereview.chromium.org/10831269/diff/1/chrome/common/extensions/docs/... File chrome/common/extensions/docs/server2/branch_utility.py (right): http://codereview.chromium.org/10831269/diff/1/chrome/common/extensions/docs/... chrome/common/extensions/docs/server2/branch_utility.py:30: 'trunk' or 'local', then |channel_name| will be returned unchanged. On 2012/08/13 00:53:00, kalman wrote: > "... because..." Done. http://codereview.chromium.org/10831269/diff/1/chrome/common/extensions/docs/... File chrome/common/extensions/docs/server2/handler.py (right): http://codereview.chromium.org/10831269/diff/1/chrome/common/extensions/docs/... chrome/common/extensions/docs/server2/handler.py:110: for branch in ['dev', 'beta', 'stable', 'trunk', 'local']] On 2012/08/13 00:53:00, kalman wrote: > currently only the branch utility knows what the actual branches are. Let's keep > it that way and put this method on branch utility (like GetAllBranchNumbers). Done. http://codereview.chromium.org/10831269/diff/1/chrome/common/extensions/docs/... chrome/common/extensions/docs/server2/handler.py:130: channel_name, On 2012/08/13 00:53:00, kalman wrote: > why can't this go in the constructor of template_data_source? I.e. pass into > _GetInstanceForBranch and thread through like that. > > In fact you wouldn't need to pass branch into there, you could make > _GetInstanceForBranch figure it out for itself. Done. http://codereview.chromium.org/10831269/diff/1/chrome/common/extensions/docs/... File chrome/common/extensions/docs/server2/integration_test.py (right): http://codereview.chromium.org/10831269/diff/1/chrome/common/extensions/docs/... chrome/common/extensions/docs/server2/integration_test.py:20: _FakeGetBranchNumberForChannelName) On 2012/08/13 00:53:00, kalman wrote: > I alluded to something like this in a bug report I just replied to. > > A more integration-test kind of thing to do would be to set up a fake server > which knows about the protocol. I.e. a fake omaha server kind of thing (you can > just make it return canned responses that you get from omaha). > > The trick is getting the server to actually point to that "server" instead of > the real server. > > I'd try doing this by putting the knowledge in the fake urlfetch implementation > (in appengine_wrappers). Rather than that returning a FakeUrlFetch, allow it to > be configured. I'm imagining an interface like > > > --- integration_test.py > > import appengine_wrappers > import url_constants > > class FakeOmahaProxy(object): > def fetch(self, url): > return "insert_canned_response_here" > > class FakeSubversionServer(object): > _base_pattern = re.compile(r"chrome/common/extensions/(.*)$") > def fetch(self, url): > # Only the interesting part of the path. > # ... whatever the local filesystem fetch is. > return local_fetch('chrome/common/extensions/%s' % > _base_pattern.match(url).groups()[0]) > > appengine_wrappers.ConfigureFakeUrlFetch({ > url_constants.OMAHA_PROXY_URL: FakeOmahaProxy(), > "%s/*" % url_constants.SVN_URL: FakeSubversionServer(), > }) > > > --- appengine_wrappers.py > > def ConfigureFakeUrlFetch(configuration): > FAKE_URL_FETCHER_CONFIGURATION = configuration > > try: > import ... > except ImportError: > class FakeUrlFetch(object): > def fetch(self, url): > # TODO: match globs. > return FAKE_URL_FETCHER_CONFIGURATION[url].fetch(url) > def create_rpc(self): > # Return object that eventually just calls fetch() > # so that the fake fetch implementations can handle it. > def make_fetch_call(self): > # ditto Done. http://codereview.chromium.org/10831269/diff/1/chrome/common/extensions/docs/... File chrome/common/extensions/docs/server2/template_data_source.py (right): http://codereview.chromium.org/10831269/diff/1/chrome/common/extensions/docs/... chrome/common/extensions/docs/server2/template_data_source.py:59: def Create(self, request, channel_name): On 2012/08/13 00:53:00, kalman wrote: > See comment in handler.py. This could go in the factory constructor right? Done. http://codereview.chromium.org/10831269/diff/1/chrome/common/extensions/docs/... chrome/common/extensions/docs/server2/template_data_source.py:95: self._branch_info['current'] = channel_name On 2012/08/13 00:53:00, kalman wrote: > If you do that, then you could make the current channel a parameter of > _MakeBranchDict. Done. http://codereview.chromium.org/10831269/diff/1/chrome/common/extensions/docs/... chrome/common/extensions/docs/server2/template_data_source.py:97: (('/' + channel_name) if channel_name != 'local' else '') + '/static') On 2012/08/13 00:53:00, kalman wrote: > Will this cause problems when we need to configure what empty-path points to? > > Perhaps the static resource path should be yet-another-thing to dependency > inject. This won't cause any problems. Even if the static resource is on the stable branch (what the empty path points to), there isn't a problem explicitly specifying stable as the branch. It is like this because the local branch can't be accessed just by using local for the channel name.
lgtm http://codereview.chromium.org/10831269/diff/8001/chrome/common/extensions/do... File chrome/common/extensions/docs/server2/appengine_wrappers.py (right): http://codereview.chromium.org/10831269/diff/8001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/appengine_wrappers.py:21: def GetConfiguration(key): add like if not FAKE_URL_FETCHER_CONFIGURATION: raise ValueError("No fake fetch paths have been configured. " "See ConfigureFakeUrlFetch in appengine_wrappers.py.") and then add some comment in ConfigureFakeUrlFetch. also GetConfiguration should be private, right? _GetConfiguration? http://codereview.chromium.org/10831269/diff/8001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/appengine_wrappers.py:28: """A fake urlfetch module that errors for all method calls. comment is out of date http://codereview.chromium.org/10831269/diff/8001/chrome/common/extensions/do... File chrome/common/extensions/docs/server2/template_data_source.py (right): http://codereview.chromium.org/10831269/diff/8001/chrome/common/extensions/do... chrome/common/extensions/docs/server2/template_data_source.py:16: 'branches': [ By the same logic, should rename this to _MakeChannelDict and 'branches' to 'channels'. Hm.
Had to do a little bit of merging for the apps samples patch. https://chromiumcodereview.appspot.com/10831269/diff/8001/chrome/common/exten... File chrome/common/extensions/docs/server2/appengine_wrappers.py (right): https://chromiumcodereview.appspot.com/10831269/diff/8001/chrome/common/exten... chrome/common/extensions/docs/server2/appengine_wrappers.py:21: def GetConfiguration(key): On 2012/08/13 22:36:46, kalman wrote: > add like > > if not FAKE_URL_FETCHER_CONFIGURATION: > raise ValueError("No fake fetch paths have been configured. " > "See ConfigureFakeUrlFetch in appengine_wrappers.py.") > > and then add some comment in ConfigureFakeUrlFetch. > > also GetConfiguration should be private, right? _GetConfiguration? Done. https://chromiumcodereview.appspot.com/10831269/diff/8001/chrome/common/exten... chrome/common/extensions/docs/server2/appengine_wrappers.py:28: """A fake urlfetch module that errors for all method calls. On 2012/08/13 22:36:46, kalman wrote: > comment is out of date Done. https://chromiumcodereview.appspot.com/10831269/diff/8001/chrome/common/exten... File chrome/common/extensions/docs/server2/template_data_source.py (right): https://chromiumcodereview.appspot.com/10831269/diff/8001/chrome/common/exten... chrome/common/extensions/docs/server2/template_data_source.py:16: 'branches': [ On 2012/08/13 22:36:46, kalman wrote: > By the same logic, should rename this to _MakeChannelDict and 'branches' to > 'channels'. Hm. Done.
Oops, forgot to rename some things, doing that now.
Actually, the index.html pages just needed to include the right footer. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
