Issue 355093002: Consolidate attachment URL construction.

Issue 355093002: Consolidate attachment URL construction. (Closed)

Created:
6 years, 6 months ago by maniscalco

Modified:
6 years, 6 months ago

Reviewers:
pavely

CC:
chromium-reviews, tim+watch_chromium.org, haitaol+watch_chromium.org, zea+watch_chromium.org, maniscalco+watch_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Project:
chromium

Visibility:
Public.

More Reviews

Description

Consolidate attachment URL construction. Fix bug where attachment URLs could end up with too many slashes. BUG= Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280193

Patch Set 2 : Use rbegin instead of back for portability. #

Total comments: 2

Patch Set 3 : Move static method declaration and definition. #

Created: 6 years, 6 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+81 lines, -47 lines)			Patch
M	chrome/browser/sync/profile_sync_components_factory_impl.cc	View		4 chunks	+1 line, -9 lines	0 comments	Download
M	sync/api/attachments/attachment_downloader.h	View		2 chunks	+2 lines, -3 lines	0 comments	Download
M	sync/api/attachments/attachment_downloader.cc	View		2 chunks	+2 lines, -2 lines	0 comments	Download
M	sync/internal_api/attachments/attachment_downloader_impl.cc	View		5 chunks	+5 lines, -9 lines	0 comments	Download
M	sync/internal_api/attachments/attachment_downloader_impl_unittest.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	sync/internal_api/attachments/attachment_uploader_impl.cc	View	1 2	4 chunks	+17 lines, -7 lines	0 comments	Download
M	sync/internal_api/attachments/attachment_uploader_impl_unittest.cc	View		7 chunks	+42 lines, -6 lines	0 comments	Download
M	sync/internal_api/public/attachments/attachment_downloader_impl.h	View		5 chunks	+4 lines, -5 lines	0 comments	Download
M	sync/internal_api/public/attachments/attachment_uploader_impl.h	View	1 2	3 chunks	+7 lines, -5 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

pavely

lgtm https://codereview.chromium.org/355093002/diff/20001/sync/internal_api/public/attachments/attachment_uploader_impl.h File sync/internal_api/public/attachments/attachment_uploader_impl.h (right): https://codereview.chromium.org/355093002/diff/20001/sync/internal_api/public/attachments/attachment_uploader_impl.h#newcode27 sync/internal_api/public/attachments/attachment_uploader_impl.h:27: static GURL GetURLForAttachmentId(const GURL& sync_service_url, nit: In chromium ...

6 years, 6 months ago (2014-06-26 21:09:03 UTC) #2

maniscalco

https://codereview.chromium.org/355093002/diff/20001/sync/internal_api/public/attachments/attachment_uploader_impl.h File sync/internal_api/public/attachments/attachment_uploader_impl.h (right): https://codereview.chromium.org/355093002/diff/20001/sync/internal_api/public/attachments/attachment_uploader_impl.h#newcode27 sync/internal_api/public/attachments/attachment_uploader_impl.h:27: static GURL GetURLForAttachmentId(const GURL& sync_service_url, On 2014/06/26 21:09:03, pavely ...

6 years, 6 months ago (2014-06-26 21:31:33 UTC) #3

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/maniscalco@chromium.org/355093002/40001

6 years, 6 months ago (2014-06-26 21:33:27 UTC) #5

leng

6 years, 6 months ago (2014-06-27 01:36:46 UTC) #7

Message was sent while issue was closed.

A revert of this CL has been created in
https://codereview.chromium.org/356953009/ by leng@chromium.org.

The reason for reverting is: Broke compile on ChromiumOS on Linux:

../../chrome/browser/sync/profile_sync_components_factory_impl.cc: In member
function 'virtual scoped_ptr<syncer::AttachmentService>
ProfileSyncComponentsFactoryImpl::CreateAttachmentService(syncer::AttachmentService::Delegate*)':
../../chrome/browser/sync/profile_sync_components_factory_impl.cc:648:44: error:
'url_prefix' was not declared in this scope
ninja: build stopped: subcommand failed.

http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

Expand Messages | Collapse Messages